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

Canonical AllocID #1190

Merged
merged 6 commits into from
Mar 2, 2020
Merged

Canonical AllocID #1190

merged 6 commits into from
Mar 2, 2020

Conversation

RalfJung
Copy link
Member

This is the Miri side of rust-lang/rust#69408.
This just ports the existing extern statics to the new system; no new shims are added.

Cc @christianpoveda

@bors

This comment has been minimized.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 28, 2020
Miri: let machine canonicalize AllocIDs

This implements the rustc side of the plan I laid out [here](rust-lang/miri#1147 (comment)).

Miri PR: rust-lang/miri#1190
bors added a commit to rust-lang/rust that referenced this pull request Mar 1, 2020
Miri: let machine canonicalize AllocIDs

This implements the rustc side of the plan I laid out [here](rust-lang/miri#1147 (comment)).

Miri PR: rust-lang/miri#1190
@RalfJung
Copy link
Member Author

RalfJung commented Mar 2, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 2, 2020

📌 Commit 59bddba has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 2, 2020

⌛ Testing commit 59bddba with merge 64bfe81...

@bors
Copy link
Contributor

bors commented Mar 2, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 64bfe81 to master...

@bors bors merged commit 64bfe81 into rust-lang:master Mar 2, 2020
@RalfJung RalfJung deleted the canonical-alloc-id branch March 2, 2020 08:27
@@ -74,7 +74,11 @@ pub struct MemoryExtra {
pub stacked_borrows: Option<stacked_borrows::MemoryExtra>,
pub intptrcast: intptrcast::MemoryExtra,

/// Mapping extern static names to their canonical allocation.
pub(crate) extern_statics: HashMap<&'static str, AllocId>,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be keyed on Symbol instead.
Also, does miri not use FxHashMaps? They're an easy speed boost for rustc but idk how relevant that is here.

(Sorry if this stuff has been discussed before in the context of miri, I haven't personally seen anything about this)

Copy link
Member Author

Choose a reason for hiding this comment

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

Miri doesn't currently use FxHashMap but probably should, yeah.

As for Symbol, I honestly doubt it's worth all the trouble that brings with it.

Copy link
Member

Choose a reason for hiding this comment

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

worth all the trouble that brings with it

You always start with Symbol for looking up things and then call .as_str() on it, the only thing that's more complex is inserting (you have to call Symbol::intern("...")) but that happens only once.

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.

4 participants