Page MenuHomePhabricator

wasm32: Respect `--no-mangle` in more locations in LLD
ClosedPublic

Authored by alexcrichton on Nov 8 2018, 3:35 PM.

Details

Summary

This commit updates a few locations that demangleItanium is called to all funnel through one location that respects the --no-mangle flag passed on the command line for linked wasm binaries. We found recently that --no-demangle was still demangling the names section of the binary, and it looks like this may be why!

Diff Detail

Repository
rL LLVM

Event Timeline

alexcrichton created this revision.Nov 8 2018, 3:35 PM
sbc100 added a comment.Nov 8 2018, 3:42 PM

Hmm, I don't see the context for this diff.. do you know about using 'arc diff' to upload? Maybe its not worth your while to setup in which case no worries.

wasm/Writer.cpp
568 ↗(On Diff #173236)

I think the idea was that the debug section should always contain de-mangled names and that --no-demangle was for the error message and what not. I don't feel strongly about this though. Do you even want mangled names in the names section?

Updated diff with more context

Do you even want mangled names in the names section?

Part of the issue is just that Rust doesn't use itanium mangling. It uses something that is similar, and will often (but not always!) demangle correctly if you pass it through an itanium demangler. It is that not always case that is problematic, and where we would prefer to just handle this from our side of the toolchain.

Er oops sorry haven't submitted too many patches before so was just using git diff, but I've tried an update with git diff -U100000 and that should have more context!

wasm/Writer.cpp
568 ↗(On Diff #173236)

Oh that makes sense! Our use case is that the Rust demangling scheme is itanium-like but not exactly. When run through the itanium demangler Rust symbols look reasonable, but they look much better when run through a Rust-specific demangler.

sbc100 added inline comments.Nov 8 2018, 4:00 PM
wasm/Writer.cpp
568 ↗(On Diff #173236)

So would the idea be that you would run a post-link tool to demangle the names section?

sbc100 added a comment.Nov 8 2018, 4:03 PM

Hmm, I wonder if you want to include rust-style demangling in lld directly?

From the GNU ld man page:

--demangle[=style]
 --no-demangle
     These options control whether to demangle symbol names in error messages and other output.  When the linker is told to
     demangle, it tries to present symbol names in a readable fashion: it strips leading underscores if they are used by the
     object file format, and converts C++ mangled symbol names into user readable names.  Different compilers have different
     mangling styles.  The optional demangling style argument can be used to choose an appropriate demangling style for your
     compiler.  The linker will demangle by default unless the environment variable COLLECT_NO_DEMANGLE is set.  These options
     may be used to override the default.

Sounds like to could add a "style" for rust maybe?

Either way, having though about it, I think we should accept this patch since saying --no-demangle is basically telling the linker its not smart enough to demangle by default, in which case the names section would look wrong too. Did you run the tests? If nothing failed with this change it points to the fact that we probably need more coverage :) lgtm, with a test.

Did you run the tests?

Er oops sorry about that, should be updated now! I don't have tons of experience with LLVM's tests though, so if you've got an idea of how to test with less duplication I can update to that!

wasm/Writer.cpp
568 ↗(On Diff #173236)

Indeed! That's what we currently have where wasm-bindgen as it's modifying the *.wasm will automatically demangle all the rust-looking names in the names section.

sbc100 accepted this revision.Nov 8 2018, 5:26 PM
sbc100 added inline comments.
test/wasm/cxx-mangling.ll
54 ↗(On Diff #173247)

I think you can use --check-prefixes=COMMON,DEMANGLE in this case to avoid duplicating all the common stuff.

Other than that lgtm.

This revision is now accepted and ready to land.Nov 8 2018, 5:26 PM
alexcrichton marked an inline comment as done.

Thanks for the tip! The test should now be deduplicated

This revision was automatically updated to reflect the committed changes.