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.

Sign up for GitHub

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

Jump to bottom

Stacked Borrows: Show stacktrace of when an item was popped #531

Closed
RalfJung opened this issue Nov 19, 2018 · 3 comments
Closed

Stacked Borrows: Show stacktrace of when an item was popped #531

RalfJung opened this issue Nov 19, 2018 · 3 comments
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement

Comments

@RalfJung
Copy link
Member

When debugging a stacked borrows failure, it would be really useful if, together with the message saying an item was not found in the stack, one could get a stacktrace saying "here is when the item got popped". That would indicate the conflicting access that invalidated the access that is right now being performed.
@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) labels Nov 19, 2018
@RalfJung RalfJung removed the C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps label Apr 8, 2019
@RalfJung
Copy link
Member Author

With -Zmiri-track-pointer-tag we have a crude approximation of this now (that only works for deterministic executions).

jonhoo added a commit to jonhoo/haphazard that referenced this issue Feb 28, 2022
@jonhoo
Don't produce temporary &mut T when retiring
2cd10be 
Or: Miri saves my butt, exhibit number 19388228.

Miri was complaining that accessing a reference obtained from
`HazardPointer::load` after a call to `retire` for the loaded value was
illegal. This should not be the case, since it's exactly what hazard
pointers are intended to guard against. Following the call-chain for
`Replaced::retire`, on a hunch I hovered over `self.ptr.as_mut` and saw
(to my horror) that its return type was `&mut T` (not `*mut T`). This is
undefined behavior, since we're trying to create a `&mut T`, which
_requires_ exclusivity, while there is an active `&T` to the same
referent. Even though it's immediately turned back into a `*mut T`,
that's enough to trigger UB. The fix is to never create the `&mut` in
the first place, and just directly get the pointer from the `NonNull`.

Normally, this error would have been caught by `NonNull::as_mut` being
`unsafe`. _But_, since we're calling `retire_ptr` in the same statement,
and wrapped that whole call in `unsafe`, I didn't realize that there
were _two_ `unsafe` calls in there, not just the one. And we weren't
meeting the safety requirements of one of those calls (`as_mut`). So,
I've hoisted out the method call on `ptr` to avoid this happening again
in the future. The move from `as_mut` to `as_ptr` also means we no
longer need `&mut` to the `NonNull` (which should have been another
clue something was wrong), so the function doesn't need `mut self`
(though it does still consume ownership).

I'll add that `-Zmiri-track-pointer-tag=<tag from error>` would also
have helped track this down (rust-lang/miri#531), but I didn't find it
until after I'd found the problem. Lesson learned for next time!
@saethlin
Copy link
Member

Would you consider this closed by #2030 ?

@RalfJung
Copy link
Member Author

Yes. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-aliasing Area: This affects the aliasing model (Stacked/Tree Borrows) C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RalfJung @saethlin