search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.81k stars 12.66k forks source link

Tracking issue for RFC 2603, "Rust Symbol Mangling (v0)" #60705

Open Centril opened 5 years ago

Centril commented 5 years ago

This is a tracking issue for the RFC "Rust Symbol Mangling (v0)" (rust-lang/rfcs#2603).

Current status:

Since #90128, you can control the mangling scheme with -C symbol-mangling-version, which can be:

(Before #90128, this flag was the nightly-only -Z symbol-mangling-version)

To test the new mangling, set RUSTFLAGS=-Csymbol-mangling-version=v0 (or change rustflags in .cargo/config.toml). Please note that only symbols from crates built with that flag will use the new mangling, and that tool support (e.g. debuggers) will be limited initially, until everything is upstreamed. However, RUST_BACKTRACE and rustfilt should work out of the box with either mangling version.

Steps:

Unresolved questions:

Desired availability of tooling:

Linux:

Distro Has versions of all tools with support?
Debian (latest stable) ?
Arch ?
Ubuntu (latest release) ?
Ubuntu (latest LTS) ?
Fedora (latest release) ?
Alpine (latest release) ?

Windows:

Windows does not have support for demangling either legacy or v0 Rust symbols and requires debuginfo to load the appropriate function name. As such, no special support is required.

macOS:

More investigation is needed to determine to what extent macOS system tools already support Rust v0 mangling.

eddyb commented 5 years ago

cc @rust-lang/compiler

I'd add this as an unresolved question: https://github.com/rust-lang/rfcs/pull/2603#issuecomment-489649554

I'd rather change the grammar now, if ever, before merging https://github.com/rust-lang/rust/pull/57967.

michaelwoerister commented 5 years ago

I think the Punycode vs UTF-8 and Encoding parameters types for function symbols questions are actually resolved. The RFC text contains a recommendation on what the resolution should look like (since before the motion to merge) and nobody objected.

eddyb commented 5 years ago

I tried the "type/value namespace" shorthands, and this is what I got, stats-wise: (apologies for having two tables, but it was hard to combine them further)

986807 symbols: sizes and compression ratios (▶)

v0 without shorthands (types use Nt and values use Nv)

legacy mw ▶ mw + compress v0 ▶ v0 + compress
min 30 25 0.97x 25 25 0.96x 25
avg 81 313 1.46x 193 318 1.47x 194
max 688 13409 14.83x 1115 13759 15.24x 1105
total 76.6MiB 295.4MiB 1.62x 182.0MiB 300.2MiB 1.64x 183.5MiB

v0 with shorthands (types use Z and values use V)

legacy mw ▶ mw + compress v0 ▶ v0 + compress
min 30 25 0.97x 25 24 0.96x 24
avg 81 313 1.46x 193 303 1.48x 184
max 688 13409 14.83x 1115 12925 14.99x 1047
total 76.6MiB 295.4MiB 1.62x 182.0MiB 285.5MiB 1.64x 173.9MiB

So that's around 5% reduction, kinda nice, but not that much.

robinmoussu commented 4 years ago

I just opened a bug in compiler-explorer because I noticed collision in the demangled name of different monomorphisation of the same function. As far as I understand, the current mangling scheme of rust still use the v1 witch doesn't contains the required information, while this proposed v2 would solved it? Did I correctly understood what the current situation is?

eddyb commented 4 years ago

@robinmoussu As per #57967, you can set RUSTFLAGS=-Zsymbol-mangling-version=v0 on nightly. EDIT: maybe we should update this tracking issue with that information?

crlf0710 commented 4 years ago

@eddyb @michaelwoerister I wonder if it's possible/feasible to put some "mark" into the mangled symbol name to indicate that this stack frame is preferred not to display in backtraces, #68336.

eddyb commented 4 years ago

Could just rely on a _ prefix or something similar, but either way I would prefer not to add something that marginally related, to the symbol mangling syntax.

eddyb commented 4 years ago

Updating the RFC's "v2" to "v0" as per https://github.com/rust-lang/rust/pull/57967#issuecomment-463596992 (should've done it back then).

eddyb commented 4 years ago

I've just submitted "[PATCH] Support the new ("v0") mangling scheme in rust-demangle.", to the gcc-patches mailing list (yes, the demangler used by binutils and gdb is in libiberty, the main copy of which is in GCC - would've been easier to contribute to it if it were separate, oh well).

If everything goes well, once that's merged we'll be able to also upstream the same implementation to lldb, Linux perf, valgrind, etc. - that's been the main blocker for rolling out the v0 mangling.

eddyb commented 4 years ago

Potential change we might want to make to const value mangling: https://github.com/rust-lang/rust/issues/61486#issuecomment-599085848.

The tricky aspect is whether we change the way _: usize is encoded today, from jp to p, or we leave it as-is so that demanglers can keep working unchanged.

EDIT: AFAIK, that situation can only be reached with #![feature(const_generics)], so we should be able to defer it to whenever we get around to #61486.

EDIT2: just checked and my 1 million symbols dataset doesn't contain a single instance of jp for an array length, so I think we can remove support for it.

eddyb commented 4 years ago

Quick update: I haven't received a response on the final patch in 2.5 months.

This means I can probably resubmit with the placeholder constant syntax removed (see my previous comment, i.e. https://github.com/rust-lang/rust/issues/60705#issuecomment-599087150), and/or maybe we can even implement some form of ADT mangling in the interim, since it doesn't seem to be that difficult (see https://github.com/rust-lang/rust/pull/71232 for something similar).

But this is disappointing and another delay in getting the new mangling scheme to be supported in native tooling, which would've ideally been all dealt with last year.

mark-i-m commented 4 years ago

Perhaps an MCP could be used to get it on the compiler team radar and push it through?

eddyb commented 4 years ago

@mark-i-m What do you mean? A Rust MCP won't make GCC devs suddenly respond to my emails.

mark-i-m commented 4 years ago

Oh wait it won't 🤯

Lol, sorry, I misunderstood.

bstrie commented 4 years ago

So support for this in rustc is completely implemented at this point, and now we're just waiting for tooling vendors to accept patches? If so, rather than waiting passively does anyone have contacts in the GCC project who we can reach out to to shepherd this along?

benesch commented 3 years ago

I've asked Ian Taylor to look at it, since he's reviewed many of the last patches to the demangler: https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557390.html If Ian is busy, perhaps we can ask Jeff Law or Jakub Jelinek, who have committed the prior patches.

eddyb commented 3 years ago

@benesch's updated version of the patch just landed :tada: (as https://github.com/gcc-mirror/gcc/commit/84096498a7bd788599d4a7ca63543fc7c297645e)

It includes ports of @varkor's 3 PRs related to const generics:

Thanks to @benesch for updating the patch, pushing it through, and for @jwakely for committing it!

benesch commented 3 years ago

And thank you, @eddyb, for doing all the heavy lifting here! I'll be very excited when we can stabilize the new mangling scheme.

bstrie commented 3 years ago

eddyb previously wrote:

If everything goes well, once that's merged we'll be able to also upstream the same implementation to lldb, Linux perf, valgrind, etc. - that's been the main blocker for rolling out the v0 mangling.

So is that the next step? I'm unsure if by "upstream" that means that the same code will need to be landed in all those projects manually, or if it's implying that those are all using libiberty and we'll just need to wait for them to upgrade their dependency.

Amanieu commented 3 years ago

In the hopes of moving forward with this, I propose the following plan:

I think that we can switch the default name mangling to v0 on nightly once the GCC patches have been backported and distros have updated their GCC package.

Amanieu commented 3 years ago

Valgrind patch: https://bugs.kde.org/show_bug.cgi?id=431306

ojeda commented 3 years ago

binutils/gdb also picked up the GCC's libiberty updates three days ago: https://github.com/bminor/binutils-gdb/commit/d750c713c9a34c8835e8e60370708cae675edb40

If we are backporting it to gcc-8 for distros etc., I guess those need backporting too.

Meanwhile, if someone needs the (trivial) backports for the latest releases (GDB 10.1, binutils 2.35.1), you can pick them at https://github.com/Rust-for-Linux/binutils-gdb

Amanieu commented 3 years ago

Hmm that's inconvenient, I was expecting binutils to use gcc's libiberty rather than vendoring a copy. Considering the situation, I think it's only worth backporting the demangler to the latest stable releases which are:

Distros maintaining older versions will just have to backport the changes themselves.

Amanieu commented 3 years ago

v0 name mangling support has been added to the GDB 10 development branch: https://sourceware.org/bugzilla/show_bug.cgi?id=27194

I think we should switch the default name mangling to v0 on nightly once GDB 10.2 is released.

bstrie commented 3 years ago

Before stabilization, do there exist benchmarks to determine if this has an effect has on compilation times? Presumably longer symbol names would suggest some negative effect on performance, so do we have an idea of how the length of an average symbol compares between the new and old schemes?

bjorn3 commented 3 years ago

I couldn't fine any past perf run. Even the original implementation PR seems to miss one. I opened https://github.com/rust-lang/rust/pull/81137 for a perf run.

bstrie commented 3 years ago

To get some quick points of comparison, I built regex and ripgrep using both schemes.

Compilation time for regex:

$ RUSTFLAGS= time cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 8.65s
        8.78 real        25.07 user         1.52 sys
$ RUSTFLAGS=-Zsymbol-mangling-version=v0 time cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 8.88s
        9.01 real        26.09 user         1.57 sys

Symbol size for regex:

$ nm libregex-old.rlib | grep -o "__Z.*$" | wc -c
822521
$ nm libregex-new.rlib | grep -o "__R.*$" | wc -c
1156947

Compilation time for ripgrep:

$ RUSTFLAGS= time cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 36.67s
       36.80 real       176.98 user        11.27 sys
$ RUSTFLAGS=-Zsymbol-mangling-version=v0 time cargo build
    Finished dev [unoptimized + debuginfo] target(s) in 37.67s
       37.80 real       178.71 user        11.40 sys

Symbol size for ripgrep:

$ nm target/debug/rg-old | grep -o "__Z.*$" | wc -c
3298005
$ nm target/debug/rg-new | grep -o "__R.*$" | wc -c
5415482

So regex sees its symbols grow by 40%, and ripgrep's symbols grow by 60%. Both take 2% longer to compile in a single trial, which is within the margin of error but worth keeping in mind.

bstrie commented 3 years ago

Looks like bjorn's perf build is ICEing in the test suite due to panics in rustc_symbol_mangling, which suggests this could use some battle-testing before stabilization. Would it be uncontroversial to make v0 the default in nightly only, without letting that default get uplifted into beta until formal stabilization has occurred?

Mark-Simulacrum commented 3 years ago

We can switch it on for (some of) our test builders, but I am opposed to enabling on nightly prior to at least a crater run and our tests passing.

estebank commented 3 years ago

Would it be uncontroversial to make v0 the default in nightly only

I've seen cases in the past of bugs not being caught until beta (or even worse, stable) because nightly wasn't using the stable algorithm. If we were going to do something along those lines I would prefer if it was only in a single platform or some other similar strategy to avoid us finding that we've introduced a bug later in the release train and have to scramble to fix it.

Amanieu commented 3 years ago

I would prefer holding back on changing the default on nightly until we at least have support for v0 in a released version of GDB.

bjorn3 commented 3 years ago

The perf run indicated significant regressions up to 10%.

Amanieu commented 3 years ago

It seems that almost all of the the runtime increase comes from LLVM_module_codegen_emit_obj and run_linker, which explains why the % increase is so high on incremental builds.

Also keep in mind that the % are regressions in instruction count, which has less impact on the wall time than it may seem since the linker is multi-threaded.

michaelwoerister commented 3 years ago

Those perf results are rather interesting. I wish we had less noisy wall-time data.

I think building the symbol names could be made faster by introducing some kind of caching, but the perf data seems to suggest that the slowdown actually comes from feeding longer symbol names to the linker. @eddyb collected some data a while ago, comparing symbol name lengths with various schemes: https://gist.github.com/eddyb/786598131525ef5adc9189a30e31c2fc

The "legacy" encoding (i.e. the current default) is indeed more compact because it can afford to throw away information about generic arguments. I think some increase in symbol name length is inevitable once we don't use the fixed size disambiguation hashes anymore. So I think we'll just have to bite the bullet in that respect.

lnicola commented 3 years ago

Switching to lld (which is planned, but currently in limbo, I guess) might also mitigate this. From what I've seen it's much faster on large amounts of debug info.

michaelwoerister commented 3 years ago

I checked the lldb box in the TODO list above. LLDB does not seem to do any demangling of Rust symbols so there is nothing to do there.

michaelwoerister commented 3 years ago

I've added resolving https://github.com/rust-lang/rust/issues/83611 to the TODO list. I'm looking forward to collecting more regression tests as we progress :)

marmeladema commented 3 years ago

GDB 10.2 has been released with the new rust demangler: https://www.gnu.org/software/gdb/news/

Does this mean we can start the crater run?

michaelwoerister commented 3 years ago

We might want to get https://github.com/rust-lang/rust/issues/83611 before that. @camelid has opened https://github.com/rust-lang/rust/pull/83767 to that end but I'm not quite sure what the state of that PR is.

camelid commented 3 years ago

We might want to get #83611 before that. @camelid has opened #83767 to that end but I'm not quite sure what the state of that PR is.

Yeah, I've been meaning to work on that again. I'll hopefully be able to get to that soon.

michaelwoerister commented 3 years ago

83767 has been merged now, thanks @camelid!

Before we do a crater run, can we make sure to add an assertion to the symbol mangling code in the compiler that checks that all symbol names generated can be demangled by the rustc-demangle crate? That would make the crater run much more useful.

asm89 commented 3 years ago

Looks like GitHub picked up a reference to this issue in the commit, but LLDB trunk now supports Rust's symbol mangling scheme (v0). @tmiasko has been working on support for the mangling scheme in LLVM's Demangle library. It's now used by LLDB.

bstrie commented 3 years ago

If v0 becomes the default, will it also become the default for the stdlib? The argument in favor of making this the default assumes that anyone who doesn't want it will be able to turn it off, but you won't be able to "turn it off" for std unless you recompile std yourself.

bjorn3 commented 3 years ago

Why would you not want it? I would assume that the old symbol mangling scheme will be removed somewhere after the switch.

bstrie commented 3 years ago

By "not want it" I just mean that there might exist people who are using tools that only understand the legacy mangling. I see no reason why anyone would want the legacy mangling on principle, but perhaps there's something preventing them from upgrading to newer versions of their tools. The question is, what is the reasonable time to wait in order to accommodate such users and give them time to upgrade? Nobody wants to delay this feature any longer than it already has been, but it's worth acknowledging that unless libstd ships two copies of its symbols somehow, making v0 the default might cause problems for users on older tools, and it's not as easy to workaround as I, at least, originally thought.

michaelwoerister commented 3 years ago

@tmiasko has done a great job at summarizing the results of the crate run here: https://github.com/rust-lang/rust/pull/85530#issuecomment-857855379

It looks like const generics are the main blocker. @eddyb (or anyone else), do you have ideas about how to proceed there? Would it make sense to have generic support for unstable things in the mangling scheme that allows to opaquely encode things for which we don't have an official grammar yet?

eddyb commented 3 years ago

We could introduce something like p but which instead of being the trivial _ leaf, takes a hash, so we can opaquely encode types/constants/etc. that we don't currently support mangling, as their "stable hash".

Other than that, we need to actually design a structural mangling scheme for constants, and if we have that we can "just" make it official, I think.

eddyb commented 3 years ago

@bstrie keep in mind that changing the mangling scheme of std is as easy as using -Z build-std - in fact, I've recommended using env RUSTFLAGS=-Zsymbol-mangling-version=v0 cargo build -Z build-std in the past, in order to get a fully-v0 build. The toggle likely won't be on stable, though, unless there's at least active demand for it (but that is, of course, no guarantee by itself).

michaelwoerister commented 3 years ago

OK, I think it's clear that we don't want to block the mangling scheme on const generics reaching their final state, right?

I see three possibilities:

  1. Just use the existing <const> = <type> <const-data> production and rely on the <type> to give enough information to demanglers for deciding if <const-data> is a hash value. The comment on <const-data> would then be: 
    // The encoding of a constant depends on its type. Integers use their value,
// in base 16 (0-9a-f), not their memory representation. Negative integer
// values are preceded with "n". The bool value false is encoded as `0_`, true
// value as `1_`. The char constants are encoded using their Unicode scalar
// value.
// <-- new text starting here -->
// For any other type the base-16 number is an opaque hash of the constant's 
// value and cannot be demangled. Future versions of this mangling scheme
// will likely define a structural encoding of constants that does enable
// demangling complex values.
<const-data> = ["n"] {<hex-digit>} "_"
  2. Add another alternative production to the <const> production to make this more explicit: 
    <const> = <type> <const-data>
       | "H" {<hex-digit>} "_" // opaque hash of const value
       | "p" // placeholder, shown as _
       | <backref>
  3. Add a more general way of encoding things that demanglers can safely ignore.

Option (1) seems the least disruptive. Is there a reason we can't use it to bridge the time until we have a proper encoding? I also imagine that we'll encounter const values that are just too big/complex to fully encode, so having a standard way of encoding things as stable hashes seems like a good idea in any case, right?

eddyb commented 3 years ago

Honestly, I was going to offer to take a stab at defining the mangling for ADT constructors ("2." in https://github.com/rust-lang/rust/issues/61486#issuecomment-878932102), it doesn't seem much harder than adding an opaque hash-only leaf (if we do that, I would prefer it to also be available for types too, FWIW).

And on the mangler side, we do have destructure_const to use today to take apart a non-leaf constant (we already use it for printing, which legacy mangling can trigger IIRC?).

However, @oli-obk asked me to wait on valtrees (which aren't required AFAIK, except perhaps to ensure we can't ever get duplicate symbols because of miri representations... which would also be a problem with stable hashes, right?).

Option (1) seems the least disruptive. Is there a reason we can't use it to bridge the time until we have a proper encoding?

It doesn't really fit in the design I'm imagining, which would reuse e.g. the array type syntax ("A" <type>) to represent array values ("A" <const>* "E"), as mentioned in https://github.com/rust-lang/rust/issues/61486#issuecomment-830017281 - but also, demanglers don't already support it, meaning it's as much effort as having a special prefix for opaque hash-only leaves, but also it eats up most of the encoding space for no good reason.