search

salsa-rs / salsa

A generic framework for on-demand, incrementalized computation. Inspired by adapton, glimmer, and rustc's query system.
https://salsa-rs.netlify.app/
Apache License 2.0
2.13k stars 152 forks source link

Fix assertion for same DB in `DbGuard` #532

Closed MichaReiser closed 3 months ago

MichaReiser commented 3 months ago

Working on https://github.com/salsa-rs/salsa/pull/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 https://github.com/salsa-rs/salsa/issues/536

netlify[bot] commented 3 months ago

Deploy Preview for salsa-rs canceled.

Name Link
Latest commit 354dc0eff17c712636e67910065b9b1415cd6f14
Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/66a51f2a77f0680008102189
MichaReiser commented 3 months ago

@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.