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

Test for drains that shift the tail, when inline

Merged
merged 1 commit into from Jan 3, 2022

Conversation

saethlin
Copy link
Contributor

@saethlin saethlin commented Dec 19, 2021

Previously, the test suite only had one trip through the tail-shifting
code in Drain::drop, and that is in the heap state.
In the current implementation, a tail-shifting drain while in the inline
state produces potentially dangerous aliasing which is currently
accepted by default Miri and rejected with -Ztrack-raw-pointers.

Adding this test case ensures that if this ever becomes an actual
problem it will be easy to find.

jdm
jdm approved these changes Dec 20, 2021
@jdm
Copy link
Member

jdm commented Dec 20, 2021

@bors-servo r+
Thanks!

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2021

📌 Commit f79171c has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2021

Testing commit f79171c with merge 771e9e8...

bors-servo added a commit that referenced this issue Dec 20, 2021
Test for drains that shift the tail, when inline

Previously, the test suite only had one trip through the tail-shifting
code in Drain::drop, and that is in the heap state.
In the current implementation, a tail-shifting drain while in the inline
state produces potentially dangerous aliasing which is currently
accepted by default Miri and rejected with -Ztrack-raw-pointers.

Adding this test case ensures that if this ever becomes an actual
problem it will be easy to find.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2021

💔 Test failed - checks-github

@saethlin
Copy link
Contributor Author

saethlin commented Jan 3, 2022

@bors retry

Previously, the test suite only had one trip through the tail-shifting
code in Drain::drop, and that is in the heap state.
In the current implementation, a tail-shifting drain while in the inline
state produces potentially dangerous aliasing which is currently
accepted by default Miri and rejected with -Ztrack-raw-pointers.

Adding this test case ensures that if this ever becomes an actual
problem it will be easy to find.
@saethlin
Copy link
Contributor Author

saethlin commented Jan 3, 2022

The bors failure above was only because the nightly that day didn't have a miri. I was trying to prompt for a retry, but I guess I can't :(

@jdm
Copy link
Member

jdm commented Jan 3, 2022

@bors-servo retry

@jdm
Copy link
Member

jdm commented Jan 3, 2022

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2022

📌 Commit 0fced9d has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2022

Testing commit 0fced9d with merge 7cbb3b1...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2022

☀️ Test successful - checks-github
Approved by: jdm
Pushing 7cbb3b1 to master...

@bors-servo bors-servo merged commit 7cbb3b1 into servo:master Jan 3, 2022
6 checks passed
bors-servo added a commit that referenced this issue 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 added a commit that referenced this issue Feb 7, 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.
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