[PATCH] D69650: [ELF] Suggest an arbitrary C++ overload as an alternative spelling

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:705-706
+  unsigned len;
+  return def.consume_front("_Z") && !def.consumeInteger(10, len) &&
+         len == ref.size() && def.take_front(len) == ref;
+}
----------------
dblaikie wrote:
> I'm not super comfortable with this hand-written demangling (maintenance burden/code duplication, etc) - is there something in the itanium demangling library that already does this/can be reused (or refactored so it can be reused)?
One way that uses the LLVMDemangle library is to call ItaniumPartialDemangler::partialDemangle, then check whether the root node is of type KNodeType, then check its string.

The concern is that demangling every symbol table name may be slow. Similarly, in --version-script and --dynamic-list handling, we demangle all symbol table names on demand (only when extern `"C++"` is seen) because doing it unconditionally can be slow.



================
Comment at: lld/ELF/Relocations.cpp:791-794
+    symtab->forEachSymbol([&](Symbol *sym) {
+      if (canSuggestExternCXX(name, sym->getName()))
+        s = sym;
+    });
----------------
dblaikie wrote:
> If we're only going to make one suggestion, is there any way to short-circuit the forEachSymbol to stop on the first candidate found, rather than searching for a lot of candidates that'll be ignored?
> 
> (equally, should we be printing more candidates, and maybe printing the source line/file they were defined on in the error message to help people find the right one?)
Unfortunately, forEachSymbol cannot be short-circuited (D62381). It was intended for parallel execution but it is not implemented.

------ 

> (equally, should we be printing more candidates, and maybe printing the source line/file they were defined on in the error message to help people find the right one?)

We suggest the object file name: >>> defined in:
it should have provided some hint...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650