Skip to content

Don't back up past the caller when looking for an FnEntry span #2537

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 2 commits into from
Sep 24, 2022

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Sep 8, 2022

Fixes #2536

This adds a fix for the logic as well as a regression test. In the new test tests/fail/stacked_borrows/fnentry_invalidation2.rs, before this PR, we display this diagnostic:

help: <3278> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:13:5
   |
13 |     inner(&mut t);
   |     ^^^^^^^^^^^^^

Which is very misleading. It is not this call itself, but what happens within the call that invalidates the tag we want. With this PR, we get:

help: <2798> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag inside this call
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:20:13
   |
20 |     let _ = t.sli.as_mut_ptr();
   |             ^^^^^^^^^^^^^^^^^^

Which is much better.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2022

With this PR, we get:

What's still confusing me here is that I would expect a FnEntry retag to point at the beginning of a fn call.

@saethlin
Copy link
Member Author

What's still confusing me here is that I would expect a FnEntry retag to point at the beginning of a fn call.

That's not how these spans work in general. Perhaps adding the detail about the "FnEntry retag" but not suggesting that it may be above this on the stack (my brain want to say below, but I suppose based on the above we'd say above) is what's confusing? These spans always walk the stack from the top until they get to some code in the local project, because that is usually where the user can take action.

I think the ideal output here would include both a local span which indicates where the issue is in the user's code, and also the FnEntry span which points at the argument, and possibly to another library's internals. I'll update with an attempt at that soon.

@RalfJung
Copy link
Member

Conceptually, stacks always grow upwards. Have you ever tried adding something to the bottom of a stack of books? ;)

The fact that on x86 in the call stack, "higher" frames are located at "smaller" memory addresses, is entirely an implementation detail of how the stack is represented in the machine, but changes nothing about the conceptual orientation of the stack. IMO.

@RalfJung
Copy link
Member

I think the ideal output here would include both a local span which indicates where the issue is in the user's code, and also the FnEntry span which points at the argument, and possibly to another library's internals. I'll update with an attempt at that soon.

Makes sense, though we can also make this a separate PR -- now that I understand the spec and purpose of get_parent.

@saethlin saethlin added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Sep 13, 2022
@saethlin saethlin force-pushed the dont-back-up-too-far branch from e7ba7ec to d2a1782 Compare September 15, 2022 22:07
@saethlin saethlin added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Sep 15, 2022
@bors
Copy link
Contributor

bors commented Sep 20, 2022

☔ The latest upstream changes (presumably #2548) made this pull request unmergeable. Please resolve the merge conflicts.

@saethlin saethlin force-pushed the dont-back-up-too-far branch from d2a1782 to 546fdc1 Compare September 20, 2022 14:57
@saethlin saethlin force-pushed the dont-back-up-too-far branch from 7091efe to 42f64f7 Compare September 21, 2022 22:15
@RalfJung
Copy link
Member

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Sep 24, 2022

📌 Commit fdc7dc3 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 24, 2022

⌛ Testing commit fdc7dc3 with merge 3e91125...

@bors
Copy link
Contributor

bors commented Sep 24, 2022

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

@bors bors merged commit 3e91125 into rust-lang:master Sep 24, 2022
@saethlin saethlin deleted the dont-back-up-too-far branch January 15, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhelphful stacked borrows diagnostic span
4 participants