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

[MaskRay]

Summary

The definition may be mangled while an undefined reference is not.
This may come up when the reference is from a C file or the definition
misses an extern "C".

Suggest an arbitrary <source-name> that matches the undefined reference,
if such a definition exists.

ld.lld: error: undefined symbol: foo
>>> referenced by a.o:(.text+0x1)
>>> did you mean to declare foo(int) as extern "C"?
>>> defined in: a1.o

[dblaikie]

lld/ELF/Relocations.cpp

705–706

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)?

797–800

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?)

lld/test/ELF/undef-suggest-extern-cxx.s

15 ↗

(On Diff #227203)

This situation might be worth a few more words for the user.

This current phrasing makes it sounds like the person who wrote the call to foo, might've intended to call foo(int) - when they might've actually called foo(int), but with C mangling & there's a good chance the /call/ is correct (it'd be rare that someone wrote a header file intending to only use it for C++ and manage to just by accident write perfectly valid C in that header file without the intent that that file ever be included from C code) - and that it's the definition in C++ that needs to be fixed.

"did you mean to define foo(int) (or some other overload) as extern "C"?"

might be the phrasing to go for?

[MaskRay]

lld/ELF/Relocations.cpp

705–706

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.

797–800

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...

[dblaikie]

lld/ELF/Relocations.cpp

705–706

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

797–800

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))

[ruiu]

lld/test/ELF/undef-suggest-extern-cxx.s

15 ↗

(On Diff #227203)

I like that error message style. But if the user's intention was to link foo as a C function, showing that function as foo(int) is a bit confusing? How about something like this?

we found a C++ symbol that has a similar name -- did you forget to surround a declaration with 'extern "C"'?

[peter.smith]

Thanks for the update.

lld/ELF/Relocations.cpp

707

I think it will be worth defining how long is too long for a better error message, then testing some large programs with the full demangling to see if it makes a difference that anyone would be able to perceive and get frustrated with. I'd be happy with even a few seconds for a better error message.

[MaskRay]

lld/ELF/Relocations.cpp

705–706

I agree there is a leaky abstraction here - it require some knowledge about the mangling scheme. The closest I can come up with is:

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.

797–800

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.

[MaskRay]

Improve suggestion

[MaskRay]

lld/ELF/Relocations.cpp

705–706

Opinions on the demangling method to choose? 1. as-is 2. ItaniumPartialDemangler

[dblaikie]

lld/ELF/Relocations.cpp

705–706

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))

[MaskRay]

• Rename canSuggestExternCXX to canSuggestExternCForCXX
• Use ItaniumPartialDemangler.

For the record, the original approach I used was:

static bool canSuggestExternCXX(StringRef ref, StringRef def) {
  unsigned len;
  return def.consume_front("_Z") && !def.consumeInteger(10, len) &&
         len == ref.size() && def.take_front(len) == ref;
}

[dblaikie]

lld/ELF/Relocations.cpp

797–800

Not quite following the D62381 connection here - oh, a range-based rather than callback-based iteration over non-placeholder symbols? Sure. I'll leave that up to Rui & can be refactored here (before or after this is submitted)

But the map for loop before the forEachSymbol looks like it could be refactored already - and could avoid doing more suggestion work once it finds the first suggestion, since the first is likely to be as good as the last? (so no point continuing to search once you find the first)

[MaskRay]

Address review comments

[dblaikie]

This looks good to me - but probably wouldn't hurt to get @ruiu to sign off on it as well.

[peter.smith]

I'm happy with this aswell. Thanks for the updates.

[ruiu]

LGTM

lld/ELF/Relocations.cpp

709

nit: I'd add parentheses to the right-hand side.

805–806

preHint and postHint

lld/ELF/Relocations.cpp

struct Loc {
     uint64_t offset;
   };
   std::vector<Loc> locs;
   bool isWarning;
 };
 
 static std::vector<UndefinedDiag> undefs;
 
+// Check whether the definition name def is a mangled function name that matches
+// the reference name ref.
+static bool canSuggestExternCForCXX(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;
+}
+
 // Suggest an alternative spelling of an "undefined symbol" diagnostic. Returns
 // the suggested symbol, which is either in the symbol table, or in the same
 // file of sym.
 static const Symbol *getAlternativeSpelling(const Undefined &sym,
-                                            std::string &pre_hint) {
+                                            std::string &pre_hint,
+                                            std::string &post_hint) {
   // Build a map of local defined symbols.
   DenseMap<StringRef, const Symbol *> map;
   if (sym.file && !isa<SharedFile>(sym.file)) {
     for (const Symbol *s : sym.file->getSymbols())
       if (s->isLocal() && s->isDefined())
         map.try_emplace(s->getName(), s);
   }
 
if (!d.partialDemangle(name.str().c_str()))
       if (char *buf = d.getFunctionName(nullptr, nullptr)) {
         const Symbol *s = suggest(buf);
         free(buf);
         if (s) {
           pre_hint = ": extern \"C\" ";
           return s;
         }
       }
+  } else {
+    const Symbol *s = nullptr;
+    for (auto &it : map)
+      if (canSuggestExternCForCXX(name, it.first)) {
+        s = it.second;
+        break;
+      }
+    if (!s)
+      symtab->forEachSymbol([&](Symbol *sym) {
+        if (!s && canSuggestExternCForCXX(name, sym->getName()))
+          s = sym;
+      });
+    if (s) {
+      pre_hint = " to declare ";
+      post_hint = " as extern \"C\"?";
+      return s;
+    }
   }
 
   return nullptr;
 }
 
 template <class ELFT>
 static void reportUndefinedSymbol(const UndefinedDiag &undef,
                                   bool correctSpelling) {
for (UndefinedDiag::Loc l : undef.locs) {
     i++;
   }
 
   if (i < undef.locs.size())
     msg += ("\n>>> referenced " + Twine(undef.locs.size() - i) + " more times")
                .str();
 
   if (correctSpelling) {
-    std::string pre_hint = ": ";
+    std::string pre_hint = ": ", post_hint;
     if (const Symbol *corrected =
-            getAlternativeSpelling(cast<Undefined>(sym), pre_hint)) {
-      msg += "\n>>> did you mean" + pre_hint + toString(*corrected);
+            getAlternativeSpelling(cast<Undefined>(sym), pre_hint, post_hint)) {
+      msg += "\n>>> did you mean" + pre_hint + toString(*corrected) + post_hint;
       if (corrected->file)
         msg += "\n>>> defined in: " + toString(corrected->file);
     }
   }
 
   if (sym.getName().startswith("_ZTV"))
     msg += "\nthe vtable symbol may be undefined because the class is missing "
            "its key function (see https://lld.llvm.org/missingkeyfunction)";

lld/test/ELF/undef-suggest-extern-c2.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+
+## The definition is mangled while the reference is not, suggest an arbitrary
+## C++ overload.
+# RUN: echo '.globl _Z3fooi; _Z3fooi:' | llvm-mc -filetype=obj -triple=x86_64 - -o %t1.o
+# RUN: not ld.lld %t.o %t1.o -o /dev/null 2>&1 | FileCheck %s
+
+## Check that we can suggest a local definition.
+# RUN: echo '_Z3fooi: call foo' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
+# RUN: not ld.lld %t2.o -o /dev/null 2>&1 | FileCheck %s
+
+# CHECK:      error: undefined symbol: foo
+# CHECK-NEXT: >>> referenced by {{.*}}
+# CHECK-NEXT: >>> did you mean to declare foo(int) as extern "C"?
+
+## Don't suggest nested names whose base name is "foo", e.g. F::foo().
+# RUN: echo '.globl _ZN1F3fooEv; _ZN1F3fooEv:' | llvm-mc -filetype=obj -triple=x86_64 - -o %t3.o
+# RUN: not ld.lld %t.o %t3.o -o /dev/null 2>&1 | FileCheck /dev/null --implicit-check-not='did you mean'
+
+call foo