Skip to content

TB diagnostics: avoid printing irrelevant events #2888

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
Merged
merged 1 commit into from
May 31, 2023
Merged

TB diagnostics: avoid printing irrelevant events #2888

merged 1 commit into from
May 31, 2023

Conversation

Vanille-N
Copy link
Contributor

@Vanille-N Vanille-N commented May 11, 2023 •
edited
Loading

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
Copy link
Member

You wrote

raft because I think the current version of error messages can still be confusing but I'm not sure how best to resolve that

What is an example that you still consider confusing?

RalfJung
RalfJung reviewed May 12, 2023
View reviewed changes
@Vanille-N
Copy link
Contributor Author

You wrote

raft because I think the current version of error messages can still be confusing but I'm not sure how best to resolve that

What is an example that you still consider confusing?

The pattern occurs in error-range.rs:

help: the conflicting tag <TAG> was created here, in the initial state Reserved
help: the conflicting tag <TAG> later transitioned from Frozen to Disabled due to a foreign write access at offsets [0x5..0x6]

I think it should acknowledge somehow that at some point the Reserved -> Frozen transition occurred, if only to tell the user that it's definitely not relevant to the error.

@RalfJung
Copy link
Member

It could just say "transitioned to Disabled" and thus avoid the mention of the (irrelevant) frozen.

Or you could add a line like

help: (irrelevant transitions omitted)

@Vanille-N
Copy link
Contributor Author

I like the "transitioned to Disabled" version because there's no real difference between Reserved -> Frozen -> Disabled, Reserved -> Active -> Disabled and Reserved -> Active -> Frozen -> Disabled in terms of what the user needs to fix.

RalfJung
RalfJung reviewed May 12, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 12, 2023
View reviewed changes
@RalfJung
Copy link
Member

Still has the draft label, so @rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 18, 2023
@Vanille-N Vanille-N mentioned this pull request May 19, 2023
Merged
@Vanille-N Vanille-N marked this pull request as ready for review May 19, 2023 08:21
@Vanille-N
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot 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 May 19, 2023
@Vanille-N
Copy link
Contributor Author

It's probably better to get this one before #2887 , that way we'll have the correct error messages directly.

RalfJung
RalfJung reviewed May 19, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 19, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 19, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 21, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 26, 2023
View reviewed changes
RalfJung
RalfJung reviewed May 26, 2023
View reviewed changes
@RalfJung
Copy link
Member

Thanks! Can you squash the history a bit? Then we can land this.

@Vanille-N @RalfJung
more precise filtering of events
166e355 
impl of is_relevant on transitions makes for much less noise in error messages

Co-authored-by: Ralf Jung <[email protected]>
@Vanille-N
Copy link
Contributor Author

@RalfJung bump ?

@RalfJung
Copy link
Member

Sorry I didn't see this -- GH doesn't reliably send notifications on force-pushes, so please always leave a message when the next round of review is due. :)

@bors r+

@bors
Copy link
Contributor

bors commented May 31, 2023

📌 Commit 166e355 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 31, 2023

⌛ Testing commit 166e355 with merge 413f7c5...

@bors
Copy link
Contributor

bors commented May 31, 2023

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

@bors bors merged commit 413f7c5 into rust-lang:master May 31, 2023
bors added a commit that referenced this pull request Jun 3, 2023
@bors
Auto merge of #2887 - Vanille-N:tb-mut-transmute, r=RalfJung
18662da 
TB: more fail tests (mostly shared with SB)

Although it was not in the tests, `mem::transmute` works for `UnsafeCell -> &` as well.

Draft: will also introduce more test cases for cases that fail.
Draft: depends on the new error messages from #2888
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Vanille-N @RalfJung @bors @rustbot