Skip to content

Use the better FnEntry spans in protector errors #2519

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

Merged
merged 1 commit into from
Aug 31, 2022

Conversation

saethlin
Copy link
Member

Example error, from tests/fail/stacked_borrows/invalidate_against_protector1.rs:

error: Undefined Behavior: not granting access to tag <3095> because that would remove [Unique for <3099>] which is protected because it is an argument of call 943
  --> tests/fail/stacked_borrows/invalidate_against_protector1.rs:5:25
   |
5  |     let _val = unsafe { *x }; //~ ERROR: protect
   |                         ^^ not granting access to tag <3095> because that would remove [Unique for <3099>] which is protected because it is an argument of call 943
   |
   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <3095> was created by a SharedReadWrite retag at offsets [0x0..0x4]
  --> tests/fail/stacked_borrows/invalidate_against_protector1.rs:10:16
   |
10 |     let xraw = &mut x as *mut _;
   |                ^^^^^^
help: <3099> is this argument
  --> tests/fail/stacked_borrows/invalidate_against_protector1.rs:1:23
   |
1  | fn inner(x: *mut i32, _y: &mut i32) {
   |                       ^^
   = note: backtrace:
   = note: inside `inner` at tests/fail/stacked_borrows/invalidate_against_protector1.rs:5:25
note: inside `main` at tests/fail/stacked_borrows/invalidate_against_protector1.rs:12:5
  --> tests/fail/stacked_borrows/invalidate_against_protector1.rs:12:5
   |
12 |     inner(xraw, xref);
   |     ^^^^^^^^^^^^^^^^^

Benchmarks report no change, within noise.

Comment on lines +330 to +340
}).or_else(|| {
// If we didn't find a retag that created this tag, it might be the base tag of
// this allocation.
if self.history.base.0.tag() == tag {
Some((
format!("{:?} was created here, as a base tag for {:?}", tag, self.history.id),
self.history.base.1.data()
))
} else {
None
}
Copy link
Member Author

Choose a reason for hiding this comment

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

While developing this, I tried the reproducer for the current VecDeque issue:

use std::collections::VecDeque;

fn main() {
    let mut tester: VecDeque<usize> = VecDeque::with_capacity(3);

    for i in 0..3 {
        tester.push_back(i);
    }

    let _: VecDeque<_> = tester.drain(1..2).collect();
}

Which before this PR reports no additional helps, because it fails to find the creation of the tag being used in the error. Because the tag being used is the base tag, which we never store anywhere.

// this allocation.
if self.history.base.0.tag() == tag {
Some((
format!("{:?} was created here, as a base tag for {:?}", tag, self.history.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the tests emit this message, is it reachable at all?

@RalfJung
Copy link
Member

Let's land this to unblock the build against latest rustc.
@bors r+

@saethlin if you could make a follow-up PR with a test that hits the new "base tag" code path, that would be great.

@bors
Copy link
Contributor

bors commented Aug 31, 2022

📌 Commit da0d482 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 31, 2022

⌛ Testing commit da0d482 with merge da45adc...

@bors
Copy link
Contributor

bors commented Aug 31, 2022

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

@bors bors merged commit da45adc into rust-lang:master Aug 31, 2022
@saethlin
Copy link
Member Author

Will do.

bors added a commit that referenced this pull request Sep 1, 2022
Add a protector test that demonstrates the base tag diagnostic

Per #2519 (comment), this demonstrates this case for protector diagnostics:
```
help: <3131> was created here, as a base tag for alloc1623
  --> tests/fail/stacked_borrows/invalidate_against_protector3.rs:10:19
   |
10 |         let ptr = std::alloc::alloc(std::alloc::Layout::for_value(&0i32)) as *mut i32;
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
This diagnostic is inspired by what Miri used to do with rust-lang/rust#60076 (comment)
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