Skip to content

Conversation

@zetanumbers
Copy link
Contributor

@zetanumbers zetanumbers commented Oct 30, 2025

Fixes debug assertion failure as described in #141540 (comment)

Essentially failure happens during the race while decoding one ExpnId from different threads. This ICE doesn't happen with single threaded thread_pool due to early return within decode_expn_id with the same condition:

if let Some(expn_id) = ExpnId::from_hash(hash) {
return expn_id;
}

However I believe this race does not hurt because register_expn_id is pretty much idempotent:

pub fn register_expn_id(
krate: CrateNum,
local_id: ExpnIndex,
data: ExpnData,
hash: ExpnHash,
) -> ExpnId {
debug_assert!(data.parent == ExpnId::root() || krate == data.parent.krate);
let expn_id = ExpnId { krate, local_id };
HygieneData::with(|hygiene_data| {
let _old_data = hygiene_data.foreign_expn_data.insert(expn_id, data);
let _old_hash = hygiene_data.foreign_expn_hashes.insert(expn_id, hash);
debug_assert!(_old_hash.is_none() || _old_hash == Some(hash));
let _old_id = hygiene_data.expn_hash_to_expn_id.insert(hash, expn_id);
debug_assert!(_old_id.is_none() || _old_id == Some(expn_id));
});
expn_id
}

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 30, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 30, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@Kivooeo
Copy link
Member

Kivooeo commented Oct 30, 2025

cc @cjgillot

@nnethercote
Copy link
Contributor

Fixes #141540 (comment)

I'm surprised you're seeing a debug assertion failure. Did you build your own compiler?

As for the fix, it seems plausible, but I'm not familiar with this code. @Zoxc or @Mark-Simulacrum, can you evaluate this change?

@zetanumbers
Copy link
Contributor Author

I'm surprised you're seeing a debug assertion failure. Did you build your own compiler?

Yes, I did. I am trying to solve #141540, it is the most active A-parallel-compiler issue. Got this debug assertion failure while trying to reproduce that bug, so I've investigated it.

I usually keep rust.debug = true set in my bootstrap.toml file to make sure my code is good. But even if I turned it off, strangely that debug assertion kept failing even after ./x clean. Checked everything, still no clue. But anyway.

@nnethercote
Copy link
Contributor

Ok, seems reasonable.

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 3, 2025

📌 Commit 2a5d830 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 3, 2025
@Zoxc
Copy link
Contributor

Zoxc commented Nov 3, 2025

It indeed looks like expn_hash_to_expn_id is allowed to race here, so this removal looks correct.

bors added a commit that referenced this pull request Nov 3, 2025
Rollup of 8 pull requests

Successful merges:

 - #135099 (Add FileCheck annotations to mir-opt/copy-prop)
 - #145903 (Give correct suggestion for a typo in raw pointers)
 - #147520 (Port the remaining SIMD intrinsics to const-eval)
 - #148068 (rustdoc: Use configured target modifiers when collecting doctests)
 - #148099 (Prepare to move debugger discovery from compiletest to bootstrap)
 - #148268 (rustdoc: fix `--emit=dep-info` on scraped examples)
 - #148306 (Remove double check when decoding ExpnId to avoid races)
 - #148378 (Fix documentation for std::panic::update_hook)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 786d828 into rust-lang:master Nov 3, 2025
11 checks passed
@rustbot rustbot added this to the 1.93.0 milestone Nov 3, 2025
rust-timer added a commit that referenced this pull request Nov 3, 2025
Rollup merge of #148306 - zetanumbers:expn_id_decode, r=nnethercote

Remove double check when decoding ExpnId to avoid races

Fixes debug assertion failure as described in #141540 (comment)

Essentially failure happens during the race while decoding one `ExpnId` from different threads. This ICE doesn't happen with single threaded thread_pool due to early return within `decode_expn_id` with the same condition:

https://github.com/rust-lang/rust/blob/8205e6b75ec656305ac235d4726d2c7a1ddcef14/compiler/rustc_middle/src/query/on_disk_cache.rs#L605-L607

However I believe this race does not hurt because `register_expn_id` is pretty much idempotent:

https://github.com/rust-lang/rust/blob/8205e6b75ec656305ac235d4726d2c7a1ddcef14/compiler/rustc_span/src/hygiene.rs#L1397-L1413
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Nov 4, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#135099 (Add FileCheck annotations to mir-opt/copy-prop)
 - rust-lang/rust#145903 (Give correct suggestion for a typo in raw pointers)
 - rust-lang/rust#147520 (Port the remaining SIMD intrinsics to const-eval)
 - rust-lang/rust#148068 (rustdoc: Use configured target modifiers when collecting doctests)
 - rust-lang/rust#148099 (Prepare to move debugger discovery from compiletest to bootstrap)
 - rust-lang/rust#148268 (rustdoc: fix `--emit=dep-info` on scraped examples)
 - rust-lang/rust#148306 (Remove double check when decoding ExpnId to avoid races)
 - rust-lang/rust#148378 (Fix documentation for std::panic::update_hook)

r? `@ghost`
`@rustbot` modify labels: rollup
@zetanumbers zetanumbers deleted the expn_id_decode branch November 5, 2025 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants