[PATCH] D44316: [WebAssembly] Demangle symbol names for use by the browser debugger

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

LGTM if you reduce it to just the name section changes.



================
Comment at: test/wasm/cxx-symbols.ll:1
+; RUN: llc -filetype=obj %s -o %t.o
+; RUN: wasm-ld --check-signatures -o %t.wasm %t.o
----------------
sbc100 wrote:
> Maybe call this demangle.ll?
> 
> Maybe don't make foo hidden so it get exported, that way we will see if the export is mangled or not.
> 
> Perhaps check less about the output?  Just the name section?  Just the name and export section?  I'm a little torn on this since a good test should really only test one thing ,but having the full output can be nice in that you see exactly what a given change does.  But including the full yaml output also makes the tests more brittle than they need to be.
What do you think about this?  cxx-mangling.ll maybe?  name-mangling.ll?


================
Comment at: wasm/Driver.cpp:246
+        Saver.save("undefined function " +
+                   (SymName ? StringRef(*SymName) : Sym->getName()));
     SyntheticFunction *Func = make<SyntheticFunction>(Sig, StubName);
----------------
Why this change?   


================
Comment at: wasm/Symbols.cpp:186
     if (Optional<std::string> S = demangleItanium(Sym.getName()))
-      return *S;
+      return "`" + *S + "'";
   return Sym.getName();
----------------
Make this  a separate change?


================
Comment at: wasm/Writer.cpp:538
+    Optional<std::string> Name = demangleItanium(S->getName());
+    writeStr(Sub.OS, Name ? StringRef(*Name) : S->getName(), "symbol name");
   }
----------------
It seems like all the users of demangleItanium use it in this way, so perhaps its worth changing the function to have this behavior (i.e. no need to return an Optional at all and just return the original if demangling fail).  We can do that as a followup perhaps.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44316