[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:
> 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).
I agree there is a leaky abstraction here - it require some knowledge about the mangling scheme. The closest I can come up with is:

  lang=cpp
  static bool canSuggestExternCXX(StringRef ref, StringRef def) {
    llvm::ItaniumPartialDemangler d;
    if (d.partialDemangle(def.str().c_str()))
      return false;
    char *buf = d.getFunctionName(nullptr, nullptr);
    if (!buf)
      return false;
    bool ret = ref == buf;
    free(buf);
    return ret;
  }


I am not sure it is necessarily better.

The comment `def is in the form of _Z <length> <ref> <ignored>` is here to clarify the approach we take.


================
Comment at: lld/ELF/Relocations.cpp:791-794
+    symtab->forEachSymbol([&](Symbol *sym) {
+      if (canSuggestExternCXX(name, sym->getName()))
+        s = sym;
+    });
----------------
dblaikie wrote:
> 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))
forEachSymbol is defined as:

  void forEachSymbol(llvm::function_ref<void(Symbol *)> fn) {
    for (Symbol *sym : symVector)
      if (!sym->isPlaceholder())
        fn(sym);
  }

It cannot be short-circuited, but there is no thread safety issue. If you or @ruiu think https://reviews.llvm.org/D62381#1518668 looks good, I can refactor the existing forEachSymbol call sites and send a patch for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650