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

Fix assertion for same DB in DbGuard #532

Merged
merged 2 commits into from
Jul 28, 2024
Merged

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Jul 26, 2024

Working on #529 I ran into the issue that the assertion in DbGuard asserting that the DB is unchanged consistently fails.

The underlying problem is that NonNull::eq not only compares the pointer but also the metadata which is unreliable for trait object pointers

When comparing wide pointers, both the address and the metadata are tested for equality. However, note that comparing trait object pointers (*const dyn Trait) is unreliable: pointers to values of the same underlying type can compare inequal (because vtables are duplicated in multiple codegen units), and pointers to values of different underlying type can compare equal (since identical vtables can be deduplicated within a codegen unit).
source

This PR changes the assertion to use std::ptr::addr_eq which only compares the address without the metadata.

Fixes #536

@MichaReiser MichaReiser added the bug Something isn't working label Jul 26, 2024
Copy link

netlify bot commented Jul 26, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 354dc0e
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/66a51f2a77f0680008102189

src/local_state.rs Outdated Show resolved Hide resolved
@MichaReiser
Copy link
Contributor Author

MichaReiser commented Jul 27, 2024

@nikomatsakis do you think we could merge this to mitigate the immediate panic. I know you're working on a more long-term fix, but it would unblock some short-term usage and benchmarks.

Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

OK, I'm a bit confused why the metadata is different but... sounds good.

@nikomatsakis nikomatsakis added this pull request to the merge queue Jul 28, 2024
Merged via the queue into master with commit b0ee162 Jul 28, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected panic: assertion 'left= right failed: cannot change database mid-query
3 participants