[PATCH] D116279: [lld] Add support for other demanglers other than Itanium

[Luís Ferreira via Phabricator via llvm-commits]

ljmf00 added a comment.

I built and checked with x86 and experimental WASM target locally on a Linux environment.



================
Comment at: lld/Common/Strings.cpp:27
-  // mangled type name such as "Pi" (which would demangle to "int*").
-  if (!name.startswith("_Z") && !name.startswith("__Z") &&
-      !name.startswith("___Z") && !name.startswith("____Z"))
----------------
MaskRay wrote:
> ljmf00 wrote:
> > MaskRay wrote:
> > > The tests are in case `demangle(...)` is slow on non-mangled symbols. 
> > > 
> > > I have linked chrome with `--dynamic-list` specifying a `extern "C"` version node and don't see much difference, so this seems fine.
> > > 
> > What do you mean by this? Is this validation or asking to be kept as is? There are checks against this already on `llvm::demangle`. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Demangle/Demangle.cpp#L51 . This includes checks not only for Itanium encoding but also for D and Rust.
> No action needed.
> 
> See `SymbolTable::getDemangledSyms`. When `extern "C++"` is used for ld.lld --dynamic-list, every symbol is demangled. A large executable may have millions of symbols (chrome), so the performance matters.
> 
> 1559632 of 1571293 symbols in `chrome`'s symbol table starts with `_`, so the check may be redundant.
Ok, thanks for clarifying. Yes, right now this check is restrictive and redundant.


================
Comment at: lld/include/lld/Common/Strings.h:21
 namespace lld {
-// Returns a demangled C++ symbol name. If Name is not a mangled
-// name, it returns name.
-std::string demangleItanium(llvm::StringRef name);
+/// Demangle a non-Microsoft mangled symbol.
+///
----------------
MaskRay wrote:
> llvm-project/lld nearly doesn't use doxygen \param \returns. The original comment style is sufficiently clear. The new \param just adds information a user can easily infer.
Reverted. Just removed the C++ restriction.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116279/new/

https://reviews.llvm.org/D116279