Adding support for external C functions that have integer (or empty) args and/or returns #2363

emarteca · 2022-07-12T21:47:11Z

Starts addressing @#11

Implementation

Adding support for calling external C functions that have any number of integer arguments (types of integers: i8, i16, i32, i64, u8, u16, u32, u64) and an integer return type (or void).
As suggested in @#11, the libffi crate is used to dispatch the calls to external C functions.

Modifications

Main modifications are to:

helper.rs : adding a function call_and_add_external_c_fct_to_context to read the code pointer to the external C function, dispatch the call, and save the return in MIRI internal memory. Handles all conversions between MIRI and C values (using some macros, also defined in this file).
foreign_items.rs : handles the calling of call_and_add_external_c_fct_to_context in helper.rs when a foreign item is encountered. Also adds some structs to model C representations of arguments, and the signature of the external C call.

Testing

Adds tests for the following external functions which are now supported:

int tests:
- adds 2 to a provided int (no type of int specified, so autocasts)
- takes the sum of its 12 arguments (tests stack spill)
- adds 3 to a 16 bit int
- adds an i16 to an i64
- returns -10 as an unsigned int
void tests
- void function that prints from C

Code review

The code in this PR was reviewed by @maurer on another fork -- thanks!

RalfJung · 2022-07-12T22:03:33Z

Wow, that's amazing, thanks a ton. :)
(And hello from Cambridge, I'm just across the Charles from Northeastern ;)

One high-level comment based just on the PR summary: I think it'd be better to put this new functionality into new file(s) dedicated to C FFI support. That should help things be more structured.

LegNeato · 2022-07-13T05:06:02Z

This is awesome! It would be nice to have this support as optional / gated...similar to how miri has an option for isolation, it is useful to assert that no c code is called (if you take the extreme view that C = UB 😄 ) .

oli-obk

This is really cool!

src/eval.rs

src/helpers.rs

src/shims/foreign_items.rs

tests/compiletest.rs

tests/pass/external_C/int_c_tests.rs

RalfJung · 2022-07-13T16:54:06Z

This is awesome! It would be nice to have this support as optional / gated...similar to how miri has an option for isolation, it is useful to assert that no c code is called (if you take the extreme view that C = UB smile ) .

Oh definitely, this should be off-by-default.

emarteca · 2022-07-13T17:57:26Z

Hey guys, thanks for the quick and thorough review! And hello fellow Boston squad :)
I'll go through these changes and push an updated version of the code soon.

emarteca · 2022-07-14T01:21:35Z

Hey all -- thanks again for the feedback! I've just pushed an updated version of the code, with the main changes being:

removed macros: instead now just matching on the pattern and dispatching directly (and similarly for the other macro)
modified some of the error handling logic in a few cases as requested by @oli-obk
moved all the C FFI supporting functionality into its own file c_ffi_support.rs as requested by @RalfJung
updated comment style as requested by @oli-obk

I did not fix Oli's comment about supporting the external C tests only on certain platforms as I'm not sure how best to do this.

oli-obk

I think we'll limit everything to linux for now and then look into expanding the support (and CI coverage) to other platforms in separate PRs. Please set it up so that the shared object is only built on linux for now

src/machine.rs

src/shims/foreign_items.rs

RalfJung · 2022-07-15T13:41:53Z

It would be great if you could resolve those conflicts by doing a rebase over latest master, so that CI can run on this PR.

tests/fail/external_C/bad_args.rs

tests/fail/external_C/function_not_in_SO.stderr

tests/panic/external_C/incorrect_SO_file.rs

emarteca · 2022-07-16T01:16:00Z

It would be great if you could resolve those conflicts by doing a rebase over latest master, so that CI can run on this PR.

It looks like the linux build is failing because the install is trying to download a new dependency abort_on_panic but it's supposed to be in offline mode -- if this is the issue, I'm not sure how to add new dependencies to the CI build.

Error message from CI build log:

./miri install
  Installing miri v0.1.0 (/home/runner/work/miri/miri)
error: failed to compile `miri v0.1.0 (/home/runner/work/miri/miri)`, intermediate artifacts can be found at `/home/runner/work/miri/miri/target`
Caused by:
  failed to download `abort_on_panic v2.0.0`
Caused by:
  attempting to make an HTTP request, but --offline was specified

abort_on_panic is a dependency of libffi: see in Cargo.lock

src/c_ffi_support.rs

oli-obk · 2022-07-16T06:56:28Z

It looks like the linux build is failing because the install is trying to download a new dependency abort_on_panic but it's supposed to be in offline mode -- if this is the issue, I'm not sure how to add new dependencies to the CI build.

I think it has been so long since we added a dependency to miri itself, we've since made CI broken in case we do.

@RalfJung I guess we should remove the --offline from the install command and require ppl to pass it themselves if needed?

RalfJung · 2022-07-16T12:58:01Z

I guess we should remove the --offline from the install command and require ppl to pass it themselves if needed?

I think I added it because at the time I used ./miri install a lot and was annoyed that it spent so much time querying the registry for yanked packages. But maybe that got better, so, yeah, feel free to remove the flag.

oli-obk · 2022-07-16T19:15:06Z

Our test suite needs a new feature: //@only-on-host before this can work. I'll add it first thing tomorrow

emarteca · 2022-07-17T00:49:47Z

Our test suite needs a new feature: //@only-on-host before this can work. I'll add it first thing tomorrow

Thank you! :) 🙏

RalfJung · 2022-07-17T03:25:43Z

@oli-obk Can you also make the --offline change part of that PR? Would be good to leave "global infrastructure changes" out of this already large PR.

bors · 2022-07-17T11:30:03Z

☔ The latest upstream changes (presumably #2377) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2022-07-17T11:34:14Z

OK, after a rebase everything should work now

oli-obk

Please rebase instead of merge

tests/panic/external_C/incorrect_SO_file.stderr

bors · 2022-07-18T13:53:16Z

☔ The latest upstream changes (presumably #2382) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung

I am quite excited by this, but do have a bunch of comments. :)

miri

src/bin/miri.rs

src/c_ffi_support.rs

tests/external_C/test.c

tests/fail/external_C/function_not_in_SO.rs

tests/panic/external_C/incorrect_SO_file.rs

tests/pass/external_C/print_from_c.rs

src/lib.rs

src/shims/foreign_items.rs

RalfJung · 2022-08-21T13:26:27Z

src/shims/ffi_support.rs

+        // Check if host target == the session target.
+        // This check needs to happen _after_ we check if the shared object file does not
+        // export the function: we don't want to throw an error if we are just going to use
+        // one of the shims.


I disagree. In fact I think the check should be somewhere way earlier, during initiailization. Passing this flag at all for cross-interpretation is an error.

Also throw_ub is definitely wrong here. UB is when the Abstract Machine says you did something wrong. This here is just a misconfiguration of the user, similar to setting -Zmiri-preemption-rate=notafloat.

Ah ok, sounds good.

RalfJung · 2022-08-21T13:27:11Z

src/machine.rs

@@ -410,6 +413,18 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
            preemption_rate: config.preemption_rate,
            report_progress: config.report_progress,
            since_progress_report: 0,
+            external_so_lib: config.external_so_file.as_ref().map(|lib_file_path| {


I think this is the right spot to error if host != target.

bors · 2022-08-22T15:17:59Z

☔ The latest upstream changes (presumably #2441) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk

should probably squash commits again after addressing these.

I'm checking out your branch locally now to see if it actually works in x.py

src/eval.rs

src/shims/ffi_support.rs

oli-obk · 2022-08-25T07:32:34Z

src/shims/ffi_support.rs

+
+#[derive(Debug, Clone)]
+/// Enum of supported arguments to external C functions.
+pub enum CArg {


Should probably document why this intermediate enum is used instead of directly invoking ffi::args and using libffi::high::Arg immediately

oli-obk · 2022-08-25T07:57:27Z

I'm checking out your branch locally now to see if it actually works in x.py

All the tests pass! So I guess after my nits, rebase to eliminate any merge commits and squash the other commits.

bors · 2022-08-25T12:55:37Z

☔ The latest upstream changes (presumably #2448) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2022-08-26T08:51:06Z

@bors r+

bors · 2022-08-26T08:51:08Z

📌 Commit 88a7882 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-08-26T08:51:14Z

⌛ Testing commit 88a7882 with merge 6418501...

bors · 2022-08-26T09:20:49Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6418501 to master...

saethlin · 2022-08-26T14:25:59Z

.gitignore

+<<<<<<< HEAD
+=======
+<<<<<<< HEAD
+tests/extern-so/libtestlib.so
+=======
+>>>>>>> master
+>>>>>>> 58ba05a0 (C FFI support for functions with int args and returns)


Is this a merge conflict issue?

yea, I created a fix PR for this

oh no -- good catch and thanks for fixing it!

Fix merge conflict found in #2363 (comment)

RalfJung · 2022-08-26T21:31:03Z

src/machine.rs

@@ -412,6 +416,24 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
            preemption_rate: config.preemption_rate,
            report_progress: config.report_progress,
            since_progress_report: 0,
+            external_so_lib: config.external_so_file.as_ref().map(|lib_file_path| {
+                // Check if host target == the session target.
+                if option_env!("TARGET") == Some(target_triple) {


So we panic if they are equal? How does that make sense?

This is hiding a bug, the comparison doesn't actually work right now.

Fixed by #2511

RalfJung · 2022-08-31T21:32:42Z

Unless we can quickly find a solution for this licensing issue, we might have to revert this PR (or disable some of its code, at least) in order to be able to update Miri in rustc. :/

emarteca · 2022-09-01T01:53:30Z

Unless we can quickly find a solution for this licensing issue, we might have to revert this PR (or disable some of its code, at least) in order to be able to update Miri in rustc. :/

Oh yikes 😢
If this turns into a huge problem I guess I can try rebuilding this using a different crate than libffi (that is the dependency that introduces the dependency that's causing this issue).

saethlin · 2022-09-01T02:24:31Z

Have we reached out to libffi about this? Maybe there's something easy that can be done on their end?

RalfJung · 2022-09-01T07:43:03Z

Yeah I am trying that: tov/libffi-rs#58.

@emarteca what alternatives to libffi are there?

emarteca · 2022-09-01T21:39:39Z

Yeah I am trying that: tov/libffi-rs#58.

@emarteca what alternatives to libffi are there?

Hmm, libffi is apparently higher level bindings for libffi-sys, which doesn't depend on abort_on_panic, so we could potentially try using libffi-sys directly

RalfJung · 2022-09-01T22:01:22Z

Honestly it seems easier to fork libffi and remove their use of abort_on_panic.

RalfJung · 2022-09-01T22:01:45Z

But I hope tov/libffi-rs#58 won't take too long so we can avoid any such drastic steps. :)

oli-obk requested changes Jul 13, 2022

View reviewed changes

emarteca requested a review from oli-obk July 14, 2022 01:13

oli-obk requested changes Jul 14, 2022

View reviewed changes

src/machine.rs Outdated Show resolved Hide resolved

src/machine.rs Outdated Show resolved Hide resolved

src/shims/foreign_items.rs Outdated Show resolved Hide resolved

emarteca requested review from oli-obk and RalfJung July 14, 2022 22:54

oli-obk reviewed Jul 15, 2022

View reviewed changes

tests/fail/external_C/bad_args.rs Outdated Show resolved Hide resolved

oli-obk reviewed Jul 15, 2022

View reviewed changes

tests/fail/external_C/function_not_in_SO.stderr Outdated Show resolved Hide resolved

oli-obk reviewed Jul 15, 2022

View reviewed changes

tests/panic/external_C/incorrect_SO_file.rs Outdated Show resolved Hide resolved

emarteca requested a review from oli-obk July 16, 2022 00:51

oli-obk reviewed Jul 16, 2022

View reviewed changes

src/c_ffi_support.rs Outdated Show resolved Hide resolved

oli-obk requested changes Jul 18, 2022

View reviewed changes

tests/panic/external_C/incorrect_SO_file.stderr Outdated Show resolved Hide resolved

emarteca force-pushed the int-function-args-returns branch from 3c8fff0 to c5e5754 Compare July 18, 2022 20:41

RalfJung reviewed Jul 19, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jul 19, 2022

View reviewed changes

src/shims/foreign_items.rs Outdated Show resolved Hide resolved

emarteca mentioned this pull request Aug 20, 2022

Support for passing const Miri pointers to C #2498

Closed

RalfJung reviewed Aug 21, 2022

View reviewed changes

emarteca requested a review from oli-obk August 24, 2022 22:18

oli-obk requested changes Aug 25, 2022

View reviewed changes

C FFI support for functions with int args and returns

88a7882

emarteca force-pushed the int-function-args-returns branch from dfdb158 to 88a7882 Compare August 26, 2022 00:55

emarteca requested a review from oli-obk August 26, 2022 00:56

oli-obk approved these changes Aug 26, 2022

View reviewed changes

bors merged commit 6418501 into rust-lang:master Aug 26, 2022

This was referenced Aug 26, 2022

Implement a garbage collector for tags #2479

Merged

Make sleep work with isolation enabled #2506

Merged

saethlin reviewed Aug 26, 2022

View reviewed changes

oli-obk mentioned this pull request Aug 26, 2022

Fix merge conflict #2510

Merged

bors added a commit that referenced this pull request Aug 26, 2022

Auto merge of #2510 - oli-obk:merge_conflict, r=oli-obk

4ae6874

Fix merge conflict found in #2363 (comment)

RalfJung reviewed Aug 26, 2022

View reviewed changes

Adding support for external C functions that have integer (or empty) args and/or returns #2363

Adding support for external C functions that have integer (or empty) args and/or returns #2363

Conversation

emarteca commented Jul 12, 2022 • edited Loading

Implementation

Modifications

Testing

Code review

RalfJung commented Jul 12, 2022 • edited Loading

LegNeato commented Jul 13, 2022

oli-obk left a comment

Choose a reason for hiding this comment

RalfJung commented Jul 13, 2022

emarteca commented Jul 13, 2022

emarteca commented Jul 14, 2022

oli-obk left a comment

Choose a reason for hiding this comment

RalfJung commented Jul 15, 2022

emarteca commented Jul 16, 2022

oli-obk commented Jul 16, 2022

RalfJung commented Jul 16, 2022

oli-obk commented Jul 16, 2022

emarteca commented Jul 17, 2022

RalfJung commented Jul 17, 2022 • edited Loading

bors commented Jul 17, 2022

oli-obk commented Jul 17, 2022

oli-obk left a comment

Choose a reason for hiding this comment

bors commented Jul 18, 2022

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 22, 2022

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Aug 25, 2022

bors commented Aug 25, 2022

oli-obk commented Aug 26, 2022

bors commented Aug 26, 2022

bors commented Aug 26, 2022

bors commented Aug 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Aug 31, 2022

emarteca commented Sep 1, 2022 • edited Loading

saethlin commented Sep 1, 2022

RalfJung commented Sep 1, 2022

emarteca commented Sep 1, 2022

RalfJung commented Sep 1, 2022

RalfJung commented Sep 1, 2022

emarteca commented Jul 12, 2022 •

edited

Loading

RalfJung commented Jul 12, 2022 •

edited

Loading

RalfJung commented Jul 17, 2022 •

edited

Loading

emarteca commented Sep 1, 2022 •

edited

Loading