Preserve SyntaxContext for invalid/dummy spans in crate metadata
#85211
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 640cc4c3968183bd9fd8b1b8a3859f55f52083dc with merge 2c6bc9bce129b3e11194d57c3353ef9d48cba279... |
|
☀️ Try build successful - checks-actions |
|
Queued 2c6bc9bce129b3e11194d57c3353ef9d48cba279 with parent ea3068e, future comparison URL. |
|
Finished benchmarking try commit (2c6bc9bce129b3e11194d57c3353ef9d48cba279): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
|
Thanks, @Aaron1011! I'll take a look at this asap. |
|
There is a large comment in |
|
I am very impressed by that regression test! 😃 But I don't yet understand how it triggers the assertion exactly. The metadata encoder for rust/compiler/rustc_metadata/src/rmeta/encoder.rs Lines 206 to 210 in e1ff91f I tried to reconstruct where the error happens exactly but failed: Compiling
Compiling
Compiling
Compiling
I must be overlooking something. |
|
@cjgillot - We want to encode syntax contexts in incr comp Moe for proc macro crate's, since we still want access to |
|
@michaelwoerister When we compiler Note that the failing hash verification comes in the |
|
Ah yes, now I get it. Thanks for the explanation. Very subtle 😬 |
|
@Aaron1011, it would be great if you could try to make the PR description a bit more clear. The information is kind of in there but it is a very subtle issue. If you don't come up with something, that's OK too. The discussion here should allow folks to reconstruct what's going on. Otherwise, r=me 😃 |
My comment refers to the case line 262 in rustc_metadata::rmeta::encoder (GitHub won't let me tag it). I do not understand why it does not apply to partial and invalid spans. |
|
It may also be more robust to have a uniform encoding for spans: encode the SyntaxContext before the FULL/INVALID/PARTIAL tag, since we always need it. |
|
@cjgillot: Nice catch! Your suggestion sounds good to me. |
Fixes rust-lang#85197 We already preserved the `SyntaxContext` for invalid/dummy spans in the incremental cache, but we weren't doing the same for crate metadata. If an invalid (lo/hi from different files) span is written to the incremental cache, we will decode it with a 'dummy' location, but keep the original `SyntaxContext`. Since the crate metadata encoder was only checking for `DUMMY_SP` (dummy location + root `SyntaxContext`), the metadata encoder would treat it as a normal span, encoding the `SyntaxContext`. As a result, the final span encoded to the metadata would change across sessions, even if the crate itself was unchanged. This PR updates our encoding of spans in the crate metadata to mirror the encoding of spans into the incremental cache. We now always encode a `SyntaxContext`, and encode location information for spans with a non-dummy location.
Aaron1011
force-pushed
the
metadata-invalid-span
branch
from
640cc4c to
cdca3c8
Compare
|
@michaelwoerister: I've added a more detailed description of the ICE to the PR description. I've also used the approach suggested by @cjgillot, so we should always handle proc-macro |
|
Thank you both! @bors r+ rollup=never |
|
📌 Commit cdca3c8 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit cdca3c8 with merge 04d15cfcaeb19ca69668a8c48581388079ff0503... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
cc @richkadel - it looks like preserving |
|
Apparently this is a spurious failure: #83262 |
|
@bors retry |
bors
added
S-waiting-on-bors
Yes, I was just about to say that... I see the exact same failure: @Mark-Simulacrum just brought this up again also. |
|
☀️ Test successful - checks-actions |
pnkfelix
added
the
beta-nominated
|
discussed in steering meeting about incr-comp fingerprints. beta-backport approved. |
pnkfelix
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
Merged
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 11, 2021
…ulacrum [beta] backports * Disable the machine outliner by default rust-lang#86020 * Fix incorrect gating of nonterminals in key-value attributes rust-lang#85445 * Build crtbegin.o/crtend.o from source code rust-lang#85395 * Bring back x86_64-sun-solaris target to rustup rust-lang#85252 * Preserve SyntaxContext for invalid/dummy spans in crate metadata rust-lang#85211 * [beta] backport: Remove unsound TrustedRandomAccess implementations rust-lang#86222 r? `@Mark-Simulacrum`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #85197
We already preserved the
SyntaxContextfor invalid/dummy spans in theincremental cache, but we weren't doing the same for crate metadata.
If an invalid (lo/hi from different files) span is written to the
incremental cache, we will decode it with a 'dummy' location, but keep
the original
SyntaxContext. Since the crate metadata encoder was onlychecking for
DUMMY_SP(dummy location + rootSyntaxContext),the metadata encoder would treat it as a normal span, encoding the
SyntaxContext. As a result, the final span encoded to the metadatawould change across sessions, even if the crate itself was unchanged.
This could lead to an 'unstable fingerprint' ICE under the following conditions:
SyntaxContextsince the span is invalid, while the incremental cache encoder preserves theSyntaxContextDUMMY_SP(e.g. withSyntaxContext::root()). This span gets hashed into the query fingerprint. So far, this has always happened through theoptimized_mirquery.SyntaxContext. This span gets written out to the crate metadata - since it now has a valid location, we preserve itsSyntaxContext.optimized_mir- the foreign crate hash is unchanged, but we end up decoding a different span (it now ha a non-rootSyntaxContext). This results in the fingerprint changing, resulting in an ICE.This PR updates our encoding of spans in the crate metadata to mirror
the encoding of spans into the incremental cache. We now always encode a
SyntaxContext, and encode location information for spans with anon-dummy location.