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

Support getentropy on macOS as a foreign item #3098

Merged
merged 1 commit into from Oct 6, 2023

Conversation

BlackHoleFox
Copy link
Contributor

Prior this was always assumed to be accessed via dlsym shim, but in std I'm attempting to start unconditionally linking to getentropy on macOS now that Rust's platform version support allows it.

This just moves the main logic of the previous dlsym handler into an eval context extension so it can be used via both call paths. The dlsym handler is still needed as getrandom uses it.


impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriInterpCx<'mir, 'tcx> {}
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
fn getentropy(
Copy link
Member

Choose a reason for hiding this comment

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

Please move this to macos/foreign_items and call it from dlsym. It is macos-specific and should live in the macos module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I can do that. I was following the pattern of mach_timebase_info but I guess there's not enough in this file for it to be worth existing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah for time we have all the shims for all OSes in one file. Those are old shims, we've been adjusting and organizing them differently more recently. (Also, following that pattern would mean putting all getrandom shims for all OSes into shims/rand.v.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alrighty, its been moved to the macOS-specific module.

@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2023

As mentioned in the other PR, you can do the Miri changes right then and there. I think that would be preferable -- you can then entirely remove the dlsym, supporting only the direct-linking case, and we can avoid having a shim that is not being tested.

@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2023

Ah never mind, getrandom sill uses dlsym, so we should probably support both.

@BlackHoleFox
Copy link
Contributor Author

I can't remove the dlsym functionality in these same changes, it breaks the tests that use getrandom (and any downstream crates also relying on that). So it is still being tested with this current changeset. Hah you typed out faster then me.

I'm good with moving the changes over there though since that appears easier for both of us. If you think what is in this branch right now is fine I'll close this PR and cherry-pick it over to rust-lang/rust instead.

@RalfJung
Copy link
Member

RalfJung commented Oct 6, 2023

Right, the concern about tests was that the new shim (not via dlsym) is untested now. But I guess it will be tested soon and anyway it shares the code with the dlsym shim that we do test, so...

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

📌 Commit b84e7b7 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

⌛ Testing commit b84e7b7 with merge 25cf52a...

@bors
Copy link
Collaborator

bors commented Oct 6, 2023

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 25cf52a to master...

@bors bors merged commit 25cf52a into rust-lang:master Oct 6, 2023
8 checks passed
@BlackHoleFox BlackHoleFox deleted the apple-entropy branch October 6, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants