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

Simplify event selection in TB diagnostics #2867

Merged
merged 1 commit into from
May 6, 2023

Conversation

Vanille-N
Copy link
Contributor

@Vanille-N Vanille-N commented May 1, 2023

As discussed previously, getting the range from RangeMap can make the filtering of events much simpler without any user-visible diff.

See minor exception in <9d8fc00> and decide how to resolve it

  • add a boolean flag not to record events produced by deallocations ?
  • add a help: deallocation counts as an implicit write ? (Note: could be generalized to also include help: reborrow counts as an implicit read)
  • not bother and keep as-is ?
  • something else ?

src/range_map.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented May 1, 2023

See minor exception in <9d8fc00> and decide how to resolve it

I don't entirely understand what is going on here. Why does behavior change?

And is this state transition even relevant for that example? Strongly deallocated items always prohibit deallocation no matter their state (right?), so it seems like listing the state transitions makes sense for the allocation of the accessed tag <TAG> also contains the strongly protected tag <TAG> messages?

@Vanille-N
Copy link
Contributor Author

And is this state transition even relevant for that example? Strongly deallocated items always prohibit deallocation no matter their state (right?), so it seems like listing the state transitions makes sense for the allocation of the accessed tag <TAG> also contains the strongly protected tag <TAG> messages?

Correct, that transition is not relevant at all to the error, which is why I hadn't noticed that it was missing.

We have other transitions that are shown yet not useful, like Active -> Frozen if the error is an attempted Read through Disabled, I can take the opportunity to more strictly filter the relevant events.

@RalfJung
Copy link
Member

RalfJung commented May 1, 2023

That makes sense, but sounds orthogonal to this PR. And turns out this corresponds to an activation is also old, I just hadn't noticed it before. So let's leave those parts to a separate PR.

src/concurrency/data_race.rs Outdated Show resolved Hide resolved
|
LL | drop(unsafe { Box::from_raw(raw) });
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: this corresponds to an activation
Copy link
Member

Choose a reason for hiding this comment

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

What does "this corresponds to an activation" mean? This comment is more confusing than helpful I think.

src/range_map.rs Show resolved Hide resolved
@Vanille-N
Copy link
Contributor Author

There are functions in concurrency that contain 3 ranges in the same scope, I hope I didn't misinterpret the meaning of each.

@bors
Copy link
Contributor

bors commented May 5, 2023

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

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

just a few final nits :)

src/borrow_tracker/tree_borrows/tree.rs Outdated Show resolved Hide resolved
src/borrow_tracker/tree_borrows/tree.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented May 6, 2023

Oh also this is still marked as a draft. Did you even want another review?

@Vanille-N Vanille-N marked this pull request as ready for review May 6, 2023 10:39
@RalfJung
Copy link
Member

RalfJung commented May 6, 2023

Okay, please squash this into one commit and then we can land this. :)

@Vanille-N
Copy link
Contributor Author

Note that the comment you suggested on Event.transition_range was inaccurate, so I fixed it in addition to squashing.

@RalfJung
Copy link
Member

RalfJung commented May 6, 2023

Ah, thanks for pointing that out!

@Vanille-N Vanille-N force-pushed the tb-diags branch 2 times, most recently from aa0fe7c to a6505f3 Compare May 6, 2023 12:03
@RalfJung
Copy link
Member

RalfJung commented May 6, 2023

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2023

📌 Commit a6505f3 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 6, 2023

⌛ Testing commit a6505f3 with merge fd7b0b7...

@bors
Copy link
Contributor

bors commented May 6, 2023

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

@bors bors merged commit fd7b0b7 into rust-lang:master May 6, 2023
bors added a commit that referenced this pull request May 31, 2023
TB diagnostics: avoid printing irrelevant events

History contains some events that are relevant to the location but not useful to understand the error.
We can make the selection of events more precise, from only "does it affect the location" to also "is it relevant for this kind of error"

This is also the occasion to fix #2867 (comment)

[Solved] Draft: find a way for blanks in the history to not be confusing, as with the current version the history can show the creation as `Reserved` then show where it transitioned from `Frozen` to `Disabled`, but it will say nothing of the `Reserved -> Frozen` leading up to it.
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jun 3, 2023
TB diagnostics: avoid printing irrelevant events

History contains some events that are relevant to the location but not useful to understand the error.
We can make the selection of events more precise, from only "does it affect the location" to also "is it relevant for this kind of error"

This is also the occasion to fix rust-lang/miri#2867 (comment)

[Solved] Draft: find a way for blanks in the history to not be confusing, as with the current version the history can show the creation as `Reserved` then show where it transitioned from `Frozen` to `Disabled`, but it will say nothing of the `Reserved -> Frozen` leading up to it.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
TB diagnostics: avoid printing irrelevant events

History contains some events that are relevant to the location but not useful to understand the error.
We can make the selection of events more precise, from only "does it affect the location" to also "is it relevant for this kind of error"

This is also the occasion to fix rust-lang/miri#2867 (comment)

[Solved] Draft: find a way for blanks in the history to not be confusing, as with the current version the history can show the creation as `Reserved` then show where it transitioned from `Frozen` to `Disabled`, but it will say nothing of the `Reserved -> Frozen` leading up to it.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
TB diagnostics: avoid printing irrelevant events

History contains some events that are relevant to the location but not useful to understand the error.
We can make the selection of events more precise, from only "does it affect the location" to also "is it relevant for this kind of error"

This is also the occasion to fix rust-lang/miri#2867 (comment)

[Solved] Draft: find a way for blanks in the history to not be confusing, as with the current version the history can show the creation as `Reserved` then show where it transitioned from `Frozen` to `Disabled`, but it will say nothing of the `Reserved -> Frozen` leading up to it.
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