-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use Rust Symbol Mangling by default #85530
Conversation
@bors try |
⌛ Trying commit 493a43f55df7482188e83d68b8ca1bfc26bfd11a with merge ce59db7a1b1b012fb2793c4641c1bdecad7a128b... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
@craterbot run mode=build-and-test |
🔒 Error: you're not allowed to interact with this bot. 🔑 If you are a member of the Rust team and need access, add yourself to the whitelist. |
@craterbot run mode=build-and-test |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Should this get a perf run? |
As i understand, this pr does double work: it mangle and demangle at the same time if let Err(_) = rustc_demangle::try_demangle(&mangled) {
bug!("demangle error: {:?}\n{}", instance, mangled);
} So perf run will show some non related info. |
Yeah, I was debating whether it makes sense to do a perf run here but as @klensy points out, we also do the demangling check. I would be interested to see how expensive that demangling check is. But we'll need a compiler with the new mangling scheme enabled by default as baseline. We could do that in another PR. It would be great if we could do the demangling check unconditionally -- although doing it as a debug assertion would also be a big improvement. |
@michaelwoerister @tmiasko Can you share why this needs to be a build-and-test Crater run? I think just building should suffice; that builds tests too: https://github.com/rust-lang/crater/blob/aa40b144e61f7f1c6ba6a3bfc5e85b52b9e4a177/src/runner/test.rs#L233 If mangling changes introduce different runtime behavior that would seem very surprising to me and likely hard to catch reliably with crater (e.g., collisions seem likely to be pretty hard to identify unless they cause obvious linker errors). |
@Mark-Simulacrum potential interaction with mechanisms for collecting & formatting backtraces was one of motivations for opting for some runtime coverage. Generally, reducing the scope of a crater run to build only, seems fine to me. |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
ICE for encoding_rs-0.8.28:
|
Errors related to the use of unstable const generics feature with types that are currently unsupported in v0 mangling:
In logs there are two dozens of backtraces with mangled symbols (in cases where crate internally uses rustc-demangle crate without support for v0 mangling scheme). The backtraces generated for Summary of results:
The most immediate question raised by those results is related to const generics. Enabling new mangling scheme will make it harder to experiment with unstable parts of const generics. Should we wait with switching the default until new mangling reaches parity with legacy scheme? |
493a43f
to
d56c498
Compare
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
d56c498
to
083d4a3
Compare
As @tmiasko rebased this PR on #87194, I'll try rerunning crater (only on the regressed list), with the combined changes. @bors try |
⌛ Trying commit 083d4a3 with merge 2b00690a2a9987274f9d399383a581d5df1bf5df... |
☀️ Try build successful - checks-actions |
@craterbot run mode=build-and-test list:https://crater-reports.s3.amazonaws.com/pr-85530/retry-regressed-list.txt |
🚨 Error: failed to parse the command 🆘 If you have any trouble with Crater please ping |
@craterbot run mode=build-and-test crates=list:https://crater-reports.s3.amazonaws.com/pr-85530/retry-regressed-list.txt |
🚨 Error: missing desired crates: {"https://crater-reports.s3.amazonaws.com/pr-85530/retry-regressed-list.txt"} 🆘 If you have any trouble with Crater please ping |
@craterbot run mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-85530/retry-regressed-list.txt p=1 @Mark-Simulacrum if this one works (see my previous failed attempt above), the crater docs need updating AFAICT. |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
I'm not sure I'm not missing anything (I didn't open every single log), but AFAICT this is the only regression now: error: internal compiler error: compiler/rustc_symbol_mangling/src/v0.rs:693:17:
symbol_names: unsupported constant of type `f32` (Const { ty: f32, val: Value(Scalar(0x3f800000)) }) I explicitly said I don't want to support |
…woerister,oli-obk rustc_symbol_mangling: support structural constants and &str in v0. This PR should unblock rust-lang#85530 (except for float `const` generics, which AFAIK should've never worked). (cc `@tmiasko` could the rust-lang#85530 (comment) failures be retried with a quick crater "subset" run of this PR + changing the default to `v0`? Just to make sure I didn't miss anything other than the floats) The encoding is the one suggested before in e.g. rust-lang#61486 (comment), tho this PR won't by itself finish rust-lang#61486, before closing that we'd likely want to move to `@oli-obk's` "valtrees" (i.e. rust-lang#83234 and other associated work). <hr> **EDITs**: 1. switched unit/tuple/braced-with-named-fields `<const-fields>` prefixes from `"u"`/`"T"`/`""` to `"U"`/`"T"`/`"S"` to avoid the ambiguity reported by `@tmiasko` in rust-lang#87194 (comment). 2. `rustc-demangle` PR: rust-lang/rustc-demangle#55 3. RFC amendment PR: rust-lang/rfcs#3161 * also removed the grammar changes included in that PR, from this description 4. added tests (temporarily using my fork of `rustc-demangle`) <hr> r? `@michaelwoerister`
r? @michaelwoerister