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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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"))
----------------
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.


================
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.
+///
----------------
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.


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

https://reviews.llvm.org/D116279