Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miri engine: support extra function (pointer) values #62245

Merged
merged 8 commits into from
Jul 6, 2019

Conversation

RalfJung
Copy link
Member

We want to add basic support for dlsym in Miri (needed to run the latest version of getrandom). For that to work, dlsym needs to return something that can be stored in a function pointer and later called.

So we add a new ExtraFnVal type to the Machine trait, and enable Miri's memory to associate allocation IDs with such values, so that create_fn_alloc and get_fn can work on both Instance (this is used for "normal" function pointers) and ExtraFnVal.

Cc @oli-obk

@rust-highfive
Copy link
Collaborator

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2019
@zackmdavis
Copy link
Member

I don't think I'm the most qualified reviewer for this.

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned zackmdavis Jul 2, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

Oli is on vacation for two weeks. @zackmdavis do you think someone else could review?

@zackmdavis
Copy link
Member

Well, I guess I could try to become a more qualified reviewer. I'll take a look tonight.

r? @zackmdavis

@rust-highfive rust-highfive assigned zackmdavis and unassigned oli-obk Jul 2, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2019

Thanks a lot! And feel free to ask as many questions as you want. :) I think I should be able to explain all this code fairly well.

@zackmdavis
Copy link
Member

correction: tomorrow night

@bors
Copy link
Contributor

bors commented Jul 4, 2019

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

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2019

Rebased to fix conflict.

@@ -370,6 +371,16 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
}))
}

fn call_extra_fn(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
fn_val: !,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my prior understanding was that the never (!) type is never realized (and thus is the "return type" of functions that don't return, like panic!), so I'm confused about how we can use it as a function argument? If we can't get a value of !, then how can we call this function? And if we can't call this function, what is it for?

(really sorry, I told you I wasn't the most qualified reviewer 😰 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can't get a value of !, then how can we call this function? And if we can't call this function, what is it for?

We can't call this function indeed. :)
It's a trait method, and the type of fn_val is an associated type. So there might be (and, in fact, there are) other implementations of the trait that pick a non-empty type, and then call_extra_fn takes that type. For example Miri instantiates this trait, and there call_extra_fn looks like

    fn call_extra_fn(
        ecx: &mut InterpretCx<'mir, 'tcx, Self>,
        fn_val: Dlsym,
        args: &[OpTy<'tcx, Tag>],
        dest: Option<PlaceTy<'tcx, Tag>>,
        ret: Option<mir::BasicBlock>,
    ) -> InterpResult<'tcx>

Notice the Dlsym instead of the !.

By using ! in the const_eval instance, I basically opt-out of the ability (that the Machine trait offers) to have "other" function values, and because there are no "other" function values, call_extra_fn does not have to do anything. This surfaces in the type of the method as you said; so we can implement the method trivially without even panicking or so, just by matching on fn_val to "prove" to the compiler that we are in unreachable code.

Copy link
Member Author

@RalfJung RalfJung Jul 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(really sorry, I told you I wasn't the most qualified reviewer cold_sweat )

We could ask @eddyb to also take a look if you want -- but it's hard to get a time slice from his scheduler. ;)

@zackmdavis
Copy link
Member

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 5, 2019

✌️ @RalfJung can now approve this pull request

_dest: Option<PlaceTy<'tcx>>,
_ret: Option<mir::BasicBlock>,
) -> InterpResult<'tcx> {
match fn_val {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @nikomatsakis or @canndrew brought up the idea of omitting trait methods when they're uncallable due to uninhabited arguments (and presumably they could be null in the vtable and direct calls can be replaced with unreachable).

But for now this seems fine.

@eddyb
Copy link
Member

eddyb commented Jul 5, 2019

r=me if you want, most of this seems straight-forward

@RalfJung
Copy link
Member Author

RalfJung commented Jul 5, 2019

@bors r=eddyb,zackmdavis

Thanks @zackmdavis and @eddyb!

@bors
Copy link
Contributor

bors commented Jul 5, 2019

📌 Commit ceb496c has been approved by eddyb,zackmdavis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
…kmdavis

Miri engine: support extra function (pointer) values

We want to add basic support for `dlsym` in Miri (needed to run the latest version of `getrandom`). For that to work, `dlsym` needs to return *something* that can be stored in a function pointer and later called.

So we add a new `ExtraFnVal` type to the `Machine` trait, and enable Miri's memory to associate allocation IDs with such values, so that `create_fn_alloc` and `get_fn` can work on *both* `Instance` (this is used for "normal" function pointers) and `ExtraFnVal`.

Cc @oli-obk
bors added a commit that referenced this pull request Jul 5, 2019
Rollup of 8 pull requests

Successful merges:

 - #60260 (Add support for UWP targets)
 - #62151 (Update linked OpenSSL version)
 - #62245 (Miri engine: support extra function (pointer) values)
 - #62257 (forward read_c_str method from Memory to Alloc)
 - #62264 (Fix perf regression from Miri Machine trait changes)
 - #62296 (request at least ptr-size alignment from posix_memalign)
 - #62329 (Remove support for 1-token lookahead from the lexer)
 - #62377 (Add test for ICE #62375)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Jul 6, 2019
…kmdavis

Miri engine: support extra function (pointer) values

We want to add basic support for `dlsym` in Miri (needed to run the latest version of `getrandom`). For that to work, `dlsym` needs to return *something* that can be stored in a function pointer and later called.

So we add a new `ExtraFnVal` type to the `Machine` trait, and enable Miri's memory to associate allocation IDs with such values, so that `create_fn_alloc` and `get_fn` can work on *both* `Instance` (this is used for "normal" function pointers) and `ExtraFnVal`.

Cc @oli-obk
bors added a commit that referenced this pull request Jul 6, 2019
Rollup of 7 pull requests

Successful merges:

 - #62151 (Update linked OpenSSL version)
 - #62245 (Miri engine: support extra function (pointer) values)
 - #62257 (forward read_c_str method from Memory to Alloc)
 - #62264 (Fix perf regression from Miri Machine trait changes)
 - #62296 (request at least ptr-size alignment from posix_memalign)
 - #62329 (Remove support for 1-token lookahead from the lexer)
 - #62377 (Add test for ICE #62375)

Failed merges:

r? @ghost
@bors bors merged commit ceb496c into rust-lang:master Jul 6, 2019
@RalfJung RalfJung deleted the miri-extra-fn branch July 6, 2019 08:05
bors added a commit to rust-lang/miri that referenced this pull request Jul 6, 2019
use Dlsym support to implement getentropy (and better thread spawn error)

This is the Miri side of rust-lang/rust#62245.

Fixes #789
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants