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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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;
+}
----------------
MaskRay wrote:
> 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.
> 
This is only in the error case, right? We're already going to print an error to the user - so it seems we're already doing this lazily enough to not be doing it wastefully/needlessly (especially in a "Good" codepath).


================
Comment at: lld/ELF/Relocations.cpp:791-794
+    symtab->forEachSymbol([&](Symbol *sym) {
+      if (canSuggestExternCXX(name, sym->getName()))
+        s = sym;
+    });
----------------
MaskRay wrote:
> 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...
the use of forEachSymbol here isn't thread safe, since it'd racily access 's' - so perhaps forEachSymbol could be changed to allow short-circuiting (since its callers likely depend on it not being parallel for now) & a separate parallel version could be added at some point & callers who want that behavior could be migrated to it

Also, at least as a partial alternative to reduce the cost of demangling, the callback itself could short-circuit:

    for (auto &it : map)
      if (!s && canSuggestExternCXX(name, it.first))
        s = it.second;
    if (!s)
      symtab->forEachSymbol([&](Symbol *sym) {
        if (!s && canSuggestExternCXX(name, sym->getName()))
          s = sym;
      });

(unless there's a reason you would like to prefer the last result, or the result from "forEachSymbol" over the result from the map? In which case you can put the forEachSymbol before the for loop over the map (& skip the for loop over the map if you already found a result in the forEachSymbol))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650