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

[MaskRay]

Summary

When missing an extern "C" declaration, an undefined reference may be
mangled while the definition is not. Suggest the missing
extern "C" and the base name.
This kind of errors sometimes happen for FFI.

[peter.smith]

If there were many references in an overload set abcde(int); abcde(float); etc. then we'd get multiple references matched against the suggestion abcde. I don't think this isn't the end of the world, as a user can make sense of it, even if it might look obviously wrong to some.

Thinking about the reverse problem, I think we could identify top level non-nested mangled function names by looking for a prefix of _Z(number), nested names would start with _ZN(number). We could catch these, extract the (number) of characters for the C name, and end up with something like a mapping from C name -> mangled name. If an undefined "C" name matches in the table we can demangle the mangled name. Given that there can be multiple C++ names that match the C name it might be better doing in a separate function with a custom diagnostic to handle multiple matches. Something like:

error: undefined symbol: abcde
>>> referenced by {{.*}}
>>> did you mean one of the C++ overloads such as abcde(int)

In summary I think this is a reasonable low cost addition, so I'm happy to add it. Might not be too much effort to do a separate function that could give a more specific diagnostic though.

[grimar]

lld/ELF/Relocations.cpp

766

I wonder if we might want to check if (name.startswith("_Z")) first. It might save a bit of calculations and isolate the logic a bit.

767

auto -> size_t ?

[thakis]

lld/test/ELF/undef-spell-corrector.s

72 ↗

(On Diff #226984)

Hm, this output is maybe not super understandable to people not super familiar with mangling. It says "abcde" right there in "abcde(int)", what do you mean with "did you mean"? :)

Maybe 'did you mean: extern "C" abcde'?

[dblaikie]

Doing this in both directions (when referencing a C definition with a C++ mangled name, or a C++ definition with an unmangled/C name) seems important/valuable. One comes from using a C header from C++ without wrapping it in "extern "C"" (hopefully inside the header, so it can be used reliably) and the other way happens when you want to write your C API's implementation in C++ (for C interop from a C++ library) but you haven't put "extern"C"" in the declaration of those functions in the header to be used from both C and C++.

But it could be done in two patches rather than trying to cover both cases here, if that's easier.

lld/ELF/Relocations.cpp

766

Rather than demangling and stringifying the whole name, then using a heuristic to find the function name - might it be better to use LLVM's demangling APIs to retrieve the name directly? llvm::itanium_demangle::Node::getBaseName

[MaskRay]

Use ItaniumPartialDemangler

[MaskRay]

Doing this in both directions (when referencing a C definition with a C++ mangled name, or a C++ definition with an unmangled/C name) seems important/valuable. One comes from using a C header from C++ without wrapping it in "extern "C"" (hopefully inside the header, so it can be used reliably) and the other way happens when you want to write your C API's implementation in C++ (for C interop from a C++ library) but you haven't put "extern"C"" in the declaration of those functions in the header to be used from both C and C++.

I am open to both directions but was just waiting for more feedback for the other direction. Thanks for providing the motivation!

lld/ELF/Relocations.cpp

766

Thanks for the suggestion. I'll just call ItaniumPartialDemangler::getFunctionName. We can check whether the node kind is KNodeType but such optimization may not be necessary.

[MaskRay]

In D69592#1726758, @peter.smith wrote:

If there were many references in an overload set abcde(int); abcde(float); etc. then we'd get multiple references matched against the suggestion abcde. I don't think this isn't the end of the world, as a user can make sense of it, even if it might look obviously wrong to some.

Thinking about the reverse problem, I think we could identify top level non-nested mangled function names by looking for a prefix of _Z(number), nested names would start with _ZN(number). We could catch these, extract the (number) of characters for the C name, and end up with something like a mapping from C name -> mangled name. If an undefined "C" name matches in the table we can demangle the mangled name. Given that there can be multiple C++ names that match the C name it might be better doing in a separate function with a custom diagnostic to handle multiple matches. Something like:

error: undefined symbol: abcde
>>> referenced by {{.*}}
>>> did you mean one of the C++ overloads such as abcde(int)

In summary I think this is a reasonable low cost addition, so I'm happy to add it. Might not be too much effort to do a separate function that could give a more specific diagnostic though.

Thanks for the suggestion! I am motivated to do the other direction now... D69650 implements the other direction (definition is mangled; reference is not).

[thakis]

New diagnostic looks great.

lld/ELF/Relocations.cpp

766

Is "_Z" always correct? Blocks for example start with several underscores (3 maybe?). Do we need this check here?

[MaskRay]

lld/ELF/Relocations.cpp

766

I think very few if any people will complain if the blocks feature does not get a better diagnostic on an ELF platform, so we probably don't need to try one to four underscores.

[MaskRay]

• [ELF] Suggest an arbitrary C++ overload as an alternative spelling

[MaskRay]

Ping :)

[MaskRay]

@ruiu Do D69592 and D69650 look good?

[ruiu]

LGTM

lld/ELF/Relocations.cpp

773

pre_hint -> preHint (but can be just hint?)

lld/ELF/CMakeLists.txt

add_lld_library(lldELF
   Writer.cpp
 
   LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   BinaryFormat
   BitWriter
   Core
   DebugInfoDWARF
+  Demangle
   LTO
   MC
   Object
   Option
   Support
 
   LINK_LIBS
   lldCommon
   ${LLVM_PTHREAD_LIB}
 
   DEPENDS
   ELFOptionsTableGen
   ${tablegen_deps}
   )

lld/ELF/Relocations.cpp

 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "Thunks.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/Demangle/Demangle.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::support::endian;
struct UndefinedDiag {
   bool isWarning;
 };
 
 static std::vector<UndefinedDiag> undefs;
 
 // 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) {
+static const Symbol *getAlternativeSpelling(const Undefined &sym,
+                                            std::string &pre_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);
   }
 
for (size_t i = 0, e = name.size(); i != e + 1; ++i) {
     }
 
     // Delete name[i].
     newName = (name.substr(0, i) + name.substr(i + 1)).str();
     if (const Symbol *s = suggest(newName))
       return s;
   }
 
+  // The reference may be a mangled name while the definition is not. Suggest a
+  // missing extern "C".
+  if (name.startswith("_Z")) {
+    llvm::ItaniumPartialDemangler d;
+    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;
+        }
+      }
+  }
+
   return nullptr;
 }
 
 template <class ELFT>
 static void reportUndefinedSymbol(const UndefinedDiag &undef,
                                   bool correctSpelling) {
   Symbol &sym = *undef.sym;
 
for (UndefinedDiag::Loc l : undef.locs) {
     msg += sec.getObjMsg(offset);
     i++;
   }
 
   if (i < undef.locs.size())
     msg += ("\n>>> referenced " + Twine(undef.locs.size() - i) + " more times")
                .str();
 
-  if (correctSpelling)
+  if (correctSpelling) {
+    std::string pre_hint = ": ";
     if (const Symbol *corrected =
-            getAlternativeSpelling(cast<Undefined>(sym))) {
-      msg += "\n>>> did you mean: " + toString(*corrected);
+            getAlternativeSpelling(cast<Undefined>(sym), pre_hint)) {
+      msg += "\n>>> did you mean" + pre_hint + toString(*corrected);
       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)";
 
   if (undef.isWarning)
     warn(msg);
   else

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

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+
+## The reference is mangled while the definition is not, suggest a missing
+## extern "C".
+# RUN: echo 'call _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:      error: undefined symbol: foo(int)
+# CHECK-NEXT: >>> referenced by {{.*}}
+# CHECK-NEXT: >>> did you mean: extern "C" foo
+
+## Don't suggest for nested names like F::foo() and foo::foo().
+# RUN: echo 'call _ZN1F3fooEv; call _ZN3fooC1Ev' | llvm-mc -filetype=obj -triple=x86_64 - -o %t2.o
+# RUN: not ld.lld %t.o %t2.o -o /dev/null 2>&1 | FileCheck /dev/null --implicit-check-not='did you mean'
+
+.globl _start, foo
+_start:
+foo: