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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw marked an inline comment as done.
ncw added a comment.

Landing with just the name section changes (in Writer.cpp and Driver.cpp).



================
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:
> 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?
OK, I'll rename to cxx-mangling.ll


================
Comment at: wasm/Driver.cpp:246
+        Saver.save("undefined function " +
+                   (SymName ? StringRef(*SymName) : Sym->getName()));
     SyntheticFunction *Func = make<SyntheticFunction>(Sig, StubName);
----------------
sbc100 wrote:
> Why this change?   
To make it consistent - here we're demangling a symbol name and putting it in the "name" section, and in Writer.cpp we're also demangling a symbol name and putting it in a name section.

In Writer, you agreed we wanted to demangle, regardless of whether `Config->Demangle` was set... so here we should also be demangling regardless of whether the config option is set.

I've added this condition to the test, to make it clearer that it's part of the same change, and is tested in the same place.


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


================
Comment at: wasm/Writer.cpp:538
+    Optional<std::string> Name = demangleItanium(S->getName());
+    writeStr(Sub.OS, Name ? StringRef(*Name) : S->getName(), "symbol name");
   }
----------------
sbc100 wrote:
> 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.
I hummed and ha-ed about that. It has to return a std::string since the demangled name isn't cached anywhere - but you don't really want to return a std::string copy for the non-mangled symbol names (since that would be doing a string copy for every symbol name, which is potentially wasteful).

So inlining it here is the only way to avoid a silly copy in the non-mangled case.

In our codebase, we have a class StringCopyOpt that looks like this:

```
class StringCopyOpt {
  StringCopyOpt(char* s, bool own=false) : s(s), own(own) {}
  ~StringCopyOpt() { if (own) delete [] s; }
  // move ctor and operator, etc...
  char* s; bool own;
};
```

So it allows automatic cleanup, but without requiring you make a copy for every return value, just because some return values need a copy. I can't find anything similar in LLVM, but it would be ideal as the return value from demangleItanium.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44316