[PATCH] D69650: [ELF] Suggest extern "C" when the definition is mangled while an undefined reference is not

[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:
> MaskRay wrote:
> > 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.
> Opinions on the demangling method to choose? 1. as-is 2. ItaniumPartialDemangler
> 
> 
I'd still favor the ItaniumPartialDemangler, personally - though improvements to that API wouldn't hurt either (eg: could there be a non-dynamic "getFunctionName" that only gives an answer on a simple function name & otherwise returns null? (& thus it wouldn't have dynamic allocation - it'd just return a StringRef into the existing mangled name))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69650