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

[alexcrichton]

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!

[sbc100]

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

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?

[alexcrichton]

Updated diff with more context

[fitzgen]

In D54279#1292218, @sbc100 wrote:

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.

[alexcrichton]

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

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]

wasm/Writer.cpp

568

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

[sbc100]

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.

[alexcrichton]

Fix tests

[alexcrichton]

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

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]

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.

[alexcrichton]

Thanks for the tip! The test should now be deduplicated

wasm/Driver.cpp

 
     // Add a synthetic dummy for weak undefined functions.  These dummies will
     // be GC'd if not used as the target of any "call" instructions.
-    Optional<std::string> SymName = demangleItanium(Sym->getName());
-    StringRef DebugName =
-        Saver.save("undefined function " +
-                   (SymName ? StringRef(*SymName) : Sym->getName()));
+    std::string SymName = toString(*Sym);
+    StringRef DebugName = Saver.save("undefined function " + SymName);
     SyntheticFunction *Func =
         make<SyntheticFunction>(Sig, Sym->getName(), DebugName);
     Func->setBody(UnreachableFn);

wasm/Symbols.h

 // Returns a symbol name for an error message.
 std::string toString(const wasm::Symbol &Sym);
 std::string toString(wasm::Symbol::Kind Kind);
+std::string maybeDemangleSymbol(StringRef Name);
 
 } // namespace lld
 

wasm/Symbols.cpp

 void LazySymbol::fetch() { cast<ArchiveFile>(File)->addMember(&ArchiveSymbol); }
 
 std::string lld::toString(const wasm::Symbol &Sym) {
+  return lld::maybeDemangleSymbol(Sym.getName());
+}
+
+std::string lld::maybeDemangleSymbol(StringRef Name) {
   if (Config->Demangle)
-    if (Optional<std::string> S = demangleItanium(Sym.getName()))
+    if (Optional<std::string> S = demangleItanium(Name))
       return *S;
-  return Sym.getName();
+  return Name;
 }
 
 std::string lld::toString(wasm::Symbol::Kind Kind) {

wasm/Writer.cpp

   for (const Symbol *S : ImportedSymbols) {
     if (auto *F = dyn_cast<FunctionSymbol>(S)) {
       writeUleb128(Sub.OS, F->getFunctionIndex(), "func index");
-      Optional<std::string> Name = demangleItanium(F->getName());
-      writeStr(Sub.OS, Name ? StringRef(*Name) : F->getName(), "symbol name");
+      writeStr(Sub.OS, toString(*S), "symbol name");
     }
   }
   for (const InputFunction *F : InputFunctions) {

       if (!F->getDebugName().empty()) {
         writeStr(Sub.OS, F->getDebugName(), "symbol name");
       } else {
-        Optional<std::string> Name = demangleItanium(F->getName());
-        writeStr(Sub.OS, Name ? StringRef(*Name) : F->getName(), "symbol name");
+        writeStr(Sub.OS, maybeDemangleSymbol(F->getName()), "symbol name");
       }
     }
   }