Uh oh!
There was an error while loading. Please reload this page.
Account for identity substituted items in symbol mangling - #135261
Conversation
rustbot
commented
Jan 8, 2025
Some changes occurred in coverage tests. cc @Zalathar |
Zalathar
commented
Jan 9, 2025
Is there anything that coverage could potentially do differently to make this less of a hassle? For unused non-generic functions, we want to synthesize the same symbol as if that function had actually been emitted. But for unused generic functions we have a bit more latitude, since the symbol name is fundamentally fake. The biggest constraint is that it has to demangle into something that the user can understand. |
compiler-errors
commented
Jan 9, 2025
Nah, this was just an oversight on my part. I think this is really the only choice we have for unused symbols. |
| // 2. We're mangling an item with identity substs. This seems to only happen | ||
| // when generating coverage, since we try to generate coverage for unused | ||
| // items too, and if something isn't monomorphized then we necessarily don't | ||
| // have anything to substitute the instance with. |
There was a problem hiding this comment.
Note that -Cinstrument-coverage automatically implies -Csymbol-mangling-version=v0, so normally this case shouldn't arise for legacy mangling ...
... except that if you explicitly do -Cinstrument-coverage -Csymbol-mangling-version=legacy -Zunstable-options, you do end up with coverage instrumentation using legacy mangling anyway, with only a compiler warning. So this case can arise after all.
(In other words, I don't think there's anything actionable in this review comment; I just wanted to note my sadness.)
There was a problem hiding this comment.
It's nbd, I'd rather keep these in sync even if the legacy branch is never exercised.
oli-obk
commented
Jan 9, 2025
@bors r+ |
bors
commented
Jan 9, 2025
Uh oh!
There was an error while loading. Please reload this page.
lcnr
commented
Jan 9, 2025
i added a comment to the ui test @bors r=oli-obk |
bors
commented
Jan 9, 2025
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
lcnr
commented
Jan 9, 2025
if i had known it would cause ci failures i would have ignored that 😅 gonna check it out locally to fix and then force-puzsh |
91e1e70 to
b85b2c6Comparelcnr
commented
Jan 9, 2025
@bors r=oli-obk |
bors
commented
Jan 9, 2025
…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#134898 (Make it easier to run CI jobs locally) - rust-lang#135195 (Make `lit_to_mir_constant` and `lit_to_const` infallible) - rust-lang#135261 (Account for identity substituted items in symbol mangling) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#134898 (Make it easier to run CI jobs locally) - rust-lang#135195 (Make `lit_to_mir_constant` and `lit_to_const` infallible) - rust-lang#135261 (Account for identity substituted items in symbol mangling) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#135261 - compiler-errors:coverage-has-identity-substs, r=oli-obk Account for identity substituted items in symbol mangling See the inline comment. r? oli-obk Fixesrust-lang#135235
See the inline comment.
r? oli-obk
Fixes#135235