rust
rust copied to clipboard
rust-lang
sess: default to v0 symbol mangling
Closes #60705.
Rust's current mangling scheme depends on compiler internals; loses information about generic parameters (and other things) which makes for a worse experience when using external tools that need to interact with Rust symbol names; is inconsistent; and can contain . characters which aren't universally supported. Therefore, Rust has defined its own symbol mangling scheme which is defined in terms of the Rust language, not the compiler implementation; encodes information about generic parameters in a reversible way; has a consistent definition; and generates symbols that only use the characters A-Z, a-z, 0-9, and _.
Support for the new Rust symbol mangling scheme has been added to upstream tools that will need to interact with Rust symbols (e.g. debuggers).
This pull request changes the default symbol mangling scheme from the legacy scheme to the new Rust mangling scheme.
The following pull requests implemented v0 mangling in rustc (if I'm missing any, let me know):
- #57967
- #63559
- #75675
- #77452
- #77554
- #83767
- #87194
- #87789
Rust's symbol mangling scheme has support in the following external tools:
binutils/gdb(GNUlibiberty)- [PATCH] Move rust_{is_mangled,demangle_sym} to a private libiberty header. committed as https://github.com/gcc-mirror/gcc/commit/979526c9ce7bb79315f0f91fde0668a5ad8536df
- [PATCH] Simplify and generalize rust-demangle's unescaping logic. committed as https://github.com/gcc-mirror/gcc/commit/42bf58bb137992b876be37f8b2e683c49bc2abed
- [PATCH] Remove some restrictions from rust-demangle. committed as https://github.com/gcc-mirror/gcc/commit/e1cb00db670e4eb277f8315ecc1da65a5477298d
- [PATCH] Refactor rust-demangle to be independent of C++ demangling. (original submission) committed as https://github.com/gcc-mirror/gcc/commit/32fc3719e06899d43e2298ad6d0028efe5ec3024
- [PATCH] Support the new ("v0") mangling scheme in rust-demangle. (original submission) committed as https://github.com/gcc-mirror/gcc/commit/84096498a7bd788599d4a7ca63543fc7c297645e
lldb/llvm-objdump/llvm-nm/llvm-symbolizer/llvm-cxxfilt/etc- https://github.com/llvm/llvm-project/commit/7310403e3cdf8a436f94770e1a1498db05d2d091
- https://github.com/llvm/llvm-project/commit/c8c2b4629f7597ac16102dab6150da14d68167de
- https://github.com/llvm/llvm-project/commit/0a2d4f3f24a377dc7d3cbed16d30603dc27554a8
- Linux
perf valgrind
https://github.com/rust-lang/rust/pull/85530#issuecomment-857855379 contains a summary of the most recent crater run of the v0 mangling, and the remaining issues from that were fixed by #87194 (confirmed by follow-up crater run, https://github.com/rust-lang/rust/pull/85530#issuecomment-883679416).
@rustbot label +T-compiler r? @michaelwoerister
It looks like the valgrind and LLVM tool patches were only just merged in the last couple months - are they included in published releases? It'll probably take at least a year or two for them to get into e.g. LTS releases on distros, which we may not want to wait for, but having at least some release seems reasonable to me and is probably not that long a wait.
:hourglass: Trying commit 4f1bf2a84946daf85f8f7283090de7e19f76fba7 with merge d6cf5a91f80fba50e1594bddce52ee42486468c6...
It looks like the valgrind and LLVM tool patches were only just merged in the last couple months - are they included in published releases? It'll probably take at least a year or two for them to get into e.g. LTS releases on distros, which we may not want to wait for, but having at least some release seems reasonable to me and is probably not that long a wait.
As far as I can tell, the LLVM patch is in LLVM 13 (GitHub says https://github.com/llvm/llvm-project/commit/0a2d4f3f24a377dc7d3cbed16d30603dc27554a8 is on the branch at least) and valgrind 3.18.0 has support (release notes).
:sunny: Try build successful - checks-actions
Build commit: d6cf5a91f80fba50e1594bddce52ee42486468c6 (d6cf5a91f80fba50e1594bddce52ee42486468c6)
Queued d6cf5a91f80fba50e1594bddce52ee42486468c6 with parent af9b508e1d6c83a8f0e6f5c0b2b75598aa37ed27, future comparison URL.
Finished benchmarking commit (d6cf5a91f80fba50e1594bddce52ee42486468c6): comparison url.
Summary: This change led to very large relevant mixed results 🤷 in compiler performance.
- Very large improvement in instruction counts (up to -20.7% on
incr-patched: Jobbuilds ofregex) - Very large regression in instruction counts (up to 9.5% on
incr-unchangedbuilds ofwebrender-wrench)
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.
Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.
Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.
@bors rollup=never @rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression
the LLVM patch is in LLVM 13 [...] and valgrind 3.18.0 has support (release notes).
FTR, on the just released Ubuntu 21.10, the llvm available in the repos is 13.0, and the valgrind available is 3.17.0. Debian sid however is still on LLVM 11.0.
Nix OS has just updated to the new valgrind on their master branch, and I think it'll take a few days until it arrives on hydra. Stable users on 21.05 still get 3.16.1, but 21.11 seems to be on track to getting the new version.
As discussed in the compiler team triage meeting a while ago, we will do a @rust-lang/compiler team FCP on this PR. That will take at least 10 days, so that the new default would make it to stable 1.58 (released January 13th 2022). With that timeline in mind, do you still think it is necessary to wait for a valgrind release, @Mark-Simulacrum?
I think the latest valgrind release already contains the relevant patches. I'm not sure how I feel about the default being not supported by tooling on LTS distros, but I don't know that we want to wait for the potentially long time that would take -- it seems unlikely to be too impactful -- rustfilt is pretty fast and easy to install. I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact -- to maximize the time for identifying any problems we can observe in the wild from users and compiler developers using these.
Another concern I'll note that as of now this increases the compiler's install size by ~27 MB (from 378 to 405 MB) -- not a huge increase, but also not particularly small. The download size only goes up by ~5MB so I guess the extended symbols compress well. The stripped binaries avoid this extra cost, but those aren't what we ship today. Maybe it's worth blocking this on stable support for split-debuginfo, which would give a smoother path toward stripping binaries (right?) for most users.
But this is not any hard blockers IMO, just some concerns to think through as we make this decision. I think a writeup of each of these and why we're making a particular tradeoff here would be helpful to me at least. On the other hand, there are definite benefits to the new mangling -- e.g., I've been using it for some of the performance triage + improvement work I'm doing locally -- so generally I think we should move ahead.
Timeline wise, I don't know whether we expect much breakage or issues filed, but I'll note that shipping in early January probably means most of the beta/nightly cycles will be during holidays and generally we don't have as much speed. So maybe delaying 1 releases (to ~1.59 or 1.60) would be of benefit regardless.
I don't think split-debuginfo can handle linking (import/export) symbols - and I've only heard of a way to split the latter on Windows (mapping names to "ordinals").
(I sometimes "symbols" used ambiguously - the distinction I'm making is that debuginfo is non-semantic, but "linking names" are semantic and allow less wiggle room)
It would be great to keep linking names compressed (esp. since a lot of names would share very similar crate roots, path components, etc.) until some rustc-dev-using project needs to link to librustc_driver.so, or rustc ICEs and wants to print a backtrace without debuginfo available.
Sadly I'm worried platform tooling is decades behind having this kind of infrastructure directly supported, and we'd have to do a lot of it ourselves (and in brittle ways).
I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact
Yes, that sounds like a good idea. I'll open a PR shortly.
I think a writeup of each of these and why we're making a particular tradeoff here would be helpful to me at least.
What format would you like that to have? Extending the PR message maybe?
On the other hand, there are definite benefits to the new mangling
I think that's an important point: there's also a cost to keeping the old mangling scheme around. We developed the new scheme because the old one is lacking in a number of ways.
shipping in early January probably means most of the beta/nightly cycles will be during holidays
That's good point.
I think we can see how much trouble we might run into by flipping the default for the compiler itself now (perhaps even before an FCP concludes) so that we can get some sense of the impact
Implemented in PR https://github.com/rust-lang/rust/pull/90054.
Yeah, I was thinking about adding to the PR message. I skimmed the RFC but I think it didn't directly address the tradeoffs I mentioned.
Another aspect which seems like it's worth including is our plans for eventual removal of the old mangling scheme. I could also imagine that some users might be interested in an alternate mangling that is just a hash and so is as short as possible.
I don't think split-debuginfo can handle linking (import/export) symbols - and I've only heard of a way to split the latter on Windows (mapping names to "ordinals").
Hm, I guess. I think there's probably still an increase in debuginfo size with the new mangling but I suppose it's probably not as important. Seems good to call out this support / lack thereof, though.
Another aspect which seems like it's worth including is our plans for eventual removal of the old mangling scheme. I could also imagine that some users might be interested in an alternate mangling that is just a hash and so is as short as possible.
Yes, we might want to extend the new scheme with a well-defined "hash mode" for cases like this. E.g. _RH<sha256 of what the symbol_name would be in non-hash mode>.
I would also be interested in a "zstd mode" with a predefined dictionary. That might give additional compression without losing information. But we'd need to collect numbers for that and make sure zstd dictionaries are standardized enough for something like that.
But both things would need a proper RFC amendment.
I've labeled the PR as blocked for now because of @Mark-Simulacrum's point about this becoming stable right after the holiday season. I'll look into a writeup about tradeoffs some time next week.
Timeline wise, I don't know whether we expect much breakage or issues filed, but I'll note that shipping in early January probably means most of the beta/nightly cycles will be during holidays and generally we don't have as much speed. So maybe delaying 1 releases (to ~1.59 or 1.60) would be of benefit regardless.
I think this is a really good point! Perhaps it would make sense to land this change on nightly and disable it once beta branches for a few cycles? That would give us extra testing time on nightly while not putting us on a path toward shipping in 1.58 when many people would have been on holiday.
Perhaps it would make sense to land this change on nightly and disable it once beta branches for a few cycles?
I've seen this suggested several times since the RFC and it really seems the most sensible, especially with the opt-out (back to legacy) not being something we want to stabilize.
Linux perf
Do we have a reference for the patchset enabling support in linux perf? I saw un-mangled symbols locally with my perf, and went looking in the linux source tree, and it looks like there's a vendored(?) copy of a Rust demangler here - tools/perf/util/demangle-rust.c, which does not appear to have been updated for the new mangling format.
It might be that it can be built using some other library's support for demangling, but it seems like we should make the effort to get a patch in for that copy as well. It looks like the original support was committed by @dtolnay FWIW.
Do we have a reference for the patchset enabling support in linux perf?
I assumed that the tracking issue was correct on that front, I couldn't find a patch online when I did a quick search writing up this pull request, https://github.com/rust-lang/rust/issues/60705#issue-442790471.
From https://github.com/rust-lang/rust/issues/60705#issuecomment-756746102:
No changes to perf are needed: it will automatically pick up the new demangler in libiberty.
So it should work when perf is built with GCC 10.1 or newer, I'll check in the evening.
There's definitely a Rust demangler in the perf tree, though, which seems like even if it's not normally used we should still seek to update, since presumably it's needed for some users.
There's definitely a Rust demangler in the perf tree, though, which seems like even if it's not normally used we should still seek to update, since presumably it's needed for some users.
This is only used for legacy mangling scheme. The code first calls cplus_demangle from libiberty and then post-process it for Rust legacy mangling. With v0 mangling scheme (and a up-to-date libiberty) cplus_demangle will do the demangling and no further post-processing would be needed in tree.
It looks like valgrind may also not support the v0 format just yet. I had to apply this patch to make that work locally (not confident this patch is fully sufficient, either, just seems to help).
I'm a little worried that this makes it seem like we have not tested support in all of these tools for the new format; it'd be good to verify that they actually work with the new format rather than just having an attempt at doing so. I think this should be a blocker for stabilization; it may push the timeline further out, as I think requiring a release of the relevant tools (even if not a widely available release) is necessary.
Even with the patch below, I'm seeing _RINvMsb_NtCsbU1JMPJG58Q_11rustc_index7bit_setINtB6_12HybridBitSetNtNtNtCs64O7cDhhvBv_14rustc_borrowck12region_infer6values10PointIndexE5unionBH_EB17_.llvm.13830652441186817445 not get demangled in valgrind. I suspect this is because of the .llvm suffix -- presumably the libiberty demangler is not handling that well? rustfilt seems to strip it, FWIW. It seems worth investigating, as .llvm suffixed names are not unusual.
diff --git a/coregrind/m_demangle/demangle.c b/coregrind/m_demangle/demangle.c
index 16161da2a..997e6a56b 100644
--- a/coregrind/m_demangle/demangle.c
+++ b/coregrind/m_demangle/demangle.c
@@ -119,7 +119,8 @@ void VG_(demangle) ( Bool do_cxx_demangling, Bool do_z_demangling,
/* Possibly undo (1) */
if (do_cxx_demangling && VG_(clo_demangle)
- && orig != NULL && orig[0] == '_' && orig[1] == 'Z') {
+ && orig != NULL && orig[0] == '_' && (orig[1] == 'Z'
+ || orig[1] == 'R')) {
/* !!! vvv STATIC vvv !!! */
static HChar* demangled = NULL;
/* !!! ^^^ STATIC ^^^ !!! */
It looks like valgrind may also not support the v0 format just yet.
I hit this yesterday as well. The one-line fix was enough for things to work for me (modulo the small number of symbols with the .llvm.<numbers> suffixes, which are a separate issue). It appears that support for v0 demangling was added to Valgrind, but the single incorrect condition in the outer layer of Valgrind's demangling code meant that the newly added code is never run. So it certainly seems like it was never tested properly, and https://bugs.kde.org/show_bug.cgi?id=431306 didn't add any tests to Valgrind.
I will fix the problem on the Valgrind side, including adding some tests.
It looks like Valgrind issues a release about once a year? Oof, bad timing. Still, I suppose that gives more time for testing...
https://bugs.kde.org/show_bug.cgi?id=445184 is for the Valgrind fix.
The '.llvm.. chars. But if LLVM is going to add such suffixes, it seems like it needs to, in some fashion.
@michaelwoerister said:
Here is a link to the relevant Itanium mangling section: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#mangling-structure I wonder if just adding something like that to the grammar would be sufficient to consider the problem solved. I'd certainly prefer if LLVM and other backends wouldn't just append random stuff to symbol names.
Discussed on 2022-01-13 during T-compiler meeting (see Zulip thread)
@rustbot label -I-compiler-nominated
