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

Fix all problems encounted with Miri -Ztag-raw-pointers #277

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

saethlin
Copy link
Contributor

@saethlin saethlin commented Feb 1, 2022

I poked at this crate with -Zmiri-tag-raw-pointers before, and I was unable to fix what I found (I just added a test case that ruled out one of my wrong ideas #271). I tried again just now and I guess I just understand better this time.

This PR fixes 3 separate pointer invalidation problems, which are detected by running MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test.

Depending on how you squint, 2 or 3 of these are rust-lang/unsafe-code-guidelines#133. The last one is probably still present even with late invalidation, because set_len does a write through a &mut.

It's unclear to me if any of these things that Miri complains about are potentially a miscompilation in rustc due to the use of LLVM noalias. But perhaps given how subtle this codebase is overall, it would be best to run the tools on their pickiest settings, even if there are a few things more like a false positive than a real problem.

Miri does not check all of Stacked Borrows (the prototype aliasing model
for Rust) without -Zmiri-tag-raw-pointers. This enables the check in CI,
and makes a few adjustments to fix places where pointers were
invalidated by construction or use of a mutable reference.
Copy link
Collaborator

@mbrubeck mbrubeck left a comment

Choose a reason for hiding this comment

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

Thank you!

@mbrubeck
Copy link
Collaborator

mbrubeck commented Feb 1, 2022

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit b257aad has been approved by mbrubeck

bors-servo added a commit that referenced this pull request Feb 1, 2022
Fix all problems encounted with Miri -Ztag-raw-pointers

I poked at this crate with `-Zmiri-tag-raw-pointers` before, and I was unable to fix what I found (I just added a test case that ruled out one of my wrong ideas #271). I tried again just now and I guess I just understand better this time.

This PR fixes 3 separate pointer invalidation problems, which are detected by running `MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test`.

Depending on how you squint, 2 or 3 of these are rust-lang/unsafe-code-guidelines#133. The last one is _probably_ still present even with late invalidation, because `set_len` does a write through a `&mut`.

It's unclear to me if any of these things that Miri complains about are potentially a miscompilation in rustc due to the use of LLVM `noalias`. But perhaps given how subtle this codebase is overall, it would be best to run the tools on their pickiest settings, even if there are a few things more like a false positive than a real problem.
@bors-servo
Copy link
Contributor

⌛ Testing commit b257aad with merge deb82ed...

@bors-servo
Copy link
Contributor

💔 Test failed - checks-github

@mbrubeck
Copy link
Collaborator

mbrubeck commented Feb 1, 2022

(We need to wait for a nightly with working MIRI, or fix our CI script to find a working nightly build automatically.)

@saethlin
Copy link
Contributor Author

saethlin commented Feb 1, 2022

To get Miri building again, it needs to support readdir64. I've been meaning to improve Miri's libc support, but don't hold your breath. I definitely do not mind waiting for a nightly where Miri builds.

@saethlin
Copy link
Contributor Author

saethlin commented Feb 7, 2022

@mbrubeck Miri is on recent nightlies, I pushed an empty commit (please squash it away) to make CI re-run 🥳

@mbrubeck
Copy link
Collaborator

mbrubeck commented Feb 7, 2022

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit ecd69b9 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit ecd69b9 with merge 8363ea8...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-github
Approved by: mbrubeck
Pushing 8363ea8 to master...

@bors-servo bors-servo merged commit 8363ea8 into servo:master Feb 7, 2022
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.

3 participants