[ELF] Print symbols with non-default versions for better "undefined symbol" diagnostics

[MaskRay]

Summary

When reporting an "undefined symbol" diagnostic:

• We don't print @ for the reference.
• We don't print @ or @@ for the definition. https://bugs.llvm.org/show_bug.cgi?id=45318

This can lead to confusing diagnostics:

// foo may be foo@v2
ld.lld: error: undefined symbol: foo
>>> referenced by t1.o:(.text+0x1)
// foo may be foo@v1 or foo@@v1
>>> did you mean: foo
>>> defined in: t.so

There are 2 ways a symbol in symtab may get truncated:

• A @@ definition may be truncated *early* by SymbolTable::insert(). The name ends with a '\0'.
• A @ definition/reference may be truncated *later* by Symbol::parseSymbolVersion(). The name ends with a '@'.

This patch detects the second case and improves the diagnostics. The first case is not improved but the second case is sufficient to make diagnostics not confusing.

[grimar]

lld/ELF/Relocations.cpp

903 ↗

(On Diff #253384)

This is OK I think. Though I wonder if it would be cleaner to add a method to Symbol like:

StringRef getFullName() const { return {nameData}; }

And just use it?

lld/test/ELF/undef-suggest-version.s

17

Probably would be reasonable to add a test for @@ case too?

[MaskRay]

Fix more problems

[MaskRay]

lld/ELF/Relocations.cpp

903 ↗

(On Diff #253384)

We need to demangle the unversioned part, then append @.

The demangler cannot process @. I changed lld::toString instead.

[grimar]

lld/ELF/Symbols.cpp

41

Though it is technically looks correct at this moment, it looks a bit hacky still. It violates the contract StringRef provides:
it is unexpected that user will access the buffer outside of its Size.

Is it a time to make toString method a member of Symbol? Since it is a global function that now relies on the assumption about internal implementation. (With that you will be able to access nameData member directly, what should be cleaner).

I also wonder what other people think. May be it just me who think it is a bit too hacky :)

46

Missing an empty line before this one.

lld/ELF/Symbols.h

24

"Print"? Perhaps "Returns" as it does not actually pring anything to an output.

lld/test/ELF/undef-suggest-version.s

7

I'd rephrase. It probably reads like we expect to see "did you mean: bar@@v1` in the error message?
Perhaps: "Check we suggest the symbol with a default version..."

39

MIssing a comment.

[MaskRay]

Improve test comments

[ruiu]

lld/ELF/Symbols.h

24

I don't think you need to put emphasis on "versioned". You can just say that this function returns a string representation for a symbol used for error reporting.

[MaskRay]

Address comments

[ruiu]

LGTM

[grimar]

lld/ELF/Symbols.cpp

41

This my concern perhaps should not be a stopper for this patch, since you have LGTM.

[MaskRay]

Clarify that we only improve diagnostics for non-default versions.

[MaskRay]

lld/ELF/Symbols.cpp

41

I have thought about the problem, but it is difficult to improve the situation.
nameSize is truncated here:

void Symbol::parseSymbolVersion() {
  StringRef s = getName();
  size_t pos = s.find('@');
  if (pos == 0 || pos == StringRef::npos)
    return;
  StringRef verstr = s.substr(pos + 1);
  if (verstr.empty())
    return;

  // Truncate the symbol name so that it doesn't include the version string.
  nameSize = pos;

lld/ELF/Symbols.h

 #include "InputFiles.h"
 #include "InputSection.h"
 #include "lld/Common/LLVM.h"
 #include "lld/Common/Strings.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/ELF.h"
 
 namespace lld {
+// Returns a string representation for a symbol for diagnostics.
 std::string toString(const elf::Symbol &);
 
 // There are two different ways to convert an Archive::Symbol to a string:
 // One for Microsoft name mangling and one for Itanium name mangling.
 // Call the functions toCOFFString and toELFString, not just toString.
 std::string toELFString(const llvm::object::Archive::Symbol &);
 
 namespace elf {

lld/ELF/Symbols.cpp

 namespace lld {
 // Returns a symbol for an error message.
 static std::string demangle(StringRef symName) {
   if (elf::config->demangle)
     return demangleItanium(symName);
   return std::string(symName);
 }
 
-std::string toString(const elf::Symbol &b) { return demangle(b.getName()); }
+std::string toString(const elf::Symbol &sym) {
+  StringRef name = sym.getName();
+  std::string ret = demangle(name);
+
+  // If sym has a non-default version, its name may have been truncated at '@'
+  // by Symbol::parseSymbolVersion(). Add the trailing part. This check is safe
+  // because every symbol name ends with '\0'.
+  if (name.data()[name.size()] == '@')
+    ret += name.data() + name.size();
+  return ret;
+}
+
 std::string toELFString(const Archive::Symbol &b) {
   return demangle(b.getName());
 }
 
 namespace elf {
 Defined *ElfSym::bss;
 Defined *ElfSym::etext1;
 Defined *ElfSym::etext2;
 Defined *ElfSym::edata1;

lld/test/ELF/undef-suggest-version.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+# RUN: echo 'v1 {bar;};' > %t.ver
+# RUN: ld.lld -shared --version-script %t.ver %t.o -o %t.so
+
+## For an unversioned undefined symbol, check we can suggest the symbol with the
+## default version.
+# RUN: echo 'call bat' | llvm-mc -filetype=obj -triple=x86_64 - -o %tdef1.o
+# RUN: not ld.lld %t.so %tdef1.o -o /dev/null 2>&1 | FileCheck --check-prefix=DEFAULT1 %s
+
+# DEFAULT1:      error: undefined symbol: bat
+# DEFAULT1-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
+# DEFAULT1-NEXT: >>> did you mean: bar{{$}}
+# DEFAULT1-NEXT: >>> defined in: {{.*}}.so
+
+## For a versioned undefined symbol, check we can suggest the symbol with the
+## default version.
+# RUN: echo '.symver bar.v2,bar@v2; call bar.v2' | llvm-mc -filetype=obj -triple=x86_64 - -o %tdef2.o
+# RUN: not ld.lld %t.so %tdef2.o -o /dev/null 2>&1 | FileCheck --check-prefix=DEFAULT2 %s
+
+# DEFAULT2:      error: undefined symbol: bar@v2
+# DEFAULT2-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
+# DEFAULT2-NEXT: >>> did you mean: bar{{$}}
+# DEFAULT2-NEXT: >>> defined in: {{.*}}.so
+
+## For an unversioned undefined symbol, check we can suggest a symbol with
+## a non-default version.
+# RUN: echo 'call foo; call _Z3fooi' | llvm-mc -filetype=obj -triple=x86_64 - -o %thidden1.o
+# RUN: not ld.lld %t.so %thidden1.o -o /dev/null 2>&1 | FileCheck --check-prefix=HIDDEN1 %s
+
+# HIDDEN1:      error: undefined symbol: foo
+# HIDDEN1-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
+# HIDDEN1-NEXT: >>> did you mean: foo@v1
+# HIDDEN1-NEXT: >>> defined in: {{.*}}.so
+# HIDDEN1-EMPTY:
+# HIDDEN1-NEXT: error: undefined symbol: foo(int)
+# HIDDEN1-NEXT: >>> referenced by {{.*}}.o:(.text+0x6)
+# HIDDEN1-NEXT: >>> did you mean: foo(int)@v1
+# HIDDEN1-NEXT: >>> defined in: {{.*}}.so
+
+## For a versioned undefined symbol, check we can suggest a symbol with
+## a different version.
+# RUN: echo '.symver foo.v2,foo@v2; call foo.v2' | llvm-mc -filetype=obj -triple=x86_64 - -o %thidden2.o
+# RUN: not ld.lld %t.so %thidden2.o -o /dev/null 2>&1 | FileCheck --check-prefix=HIDDEN2 %s
+
+# HIDDEN2:      error: undefined symbol: foo@v2
+# HIDDEN2-NEXT: >>> referenced by {{.*}}.o:(.text+0x1)
+# HIDDEN2-NEXT: >>> did you mean: foo@v1
+# HIDDEN2-NEXT: >>> defined in: {{.*}}.so
+
+## %t.so exports bar@@v1 and two VERSYM_HIDDEN symbols: foo@v1 and _Z3fooi@v1.
+.globl foo.v1, _Z3fooi.v1, bar
+.symver foo.v1,foo@v1
+.symver _Z3fooi.v1,_Z3fooi@v1
+foo.v1:
+_Z3fooi.v1:
+bar: