[LLD][COFF] Fix Bug, occurring if Symbol has no name

[j0le]

Summary

Bug: An assertion fails:

Assertion failed: isa<To>(Val) && "cast<Ty>() argument of incompatible type!", 
file C:\Users\<user>\prog\llvm\llvm-git-lld-bug\llvm\include\llvm/Support/Casting.h, line 578

Bug is triggered, if

• a map file is requested with /MAP, and
• Architekture is ARMv7, Thumb, and
• a relative jump (branch instruction) is greater than 16 MiB (2^24)

The reason for the Bug is:

• a Thunk is created for the jump
• a Symbol for the Thunk is created
  • of type DefinedSynthetic
  • in file Writer.cpp
  • in function getThunk
• the Symbol has no name
• when creating the map file, the name of the Symbol is queried
• the function Symbol::computeName of the base class Symbol casts the this pointer to type DefinedCOFF (a derived type), but the acutal type is DefinedSynthetic
• The in the llvm::cast an assertion fails

Changes:

• Modify regression test to trigger this bug
• Give the symbol pointing to the thunk a name, to fix the bug
• Add assertion, that only DefinedCOFF symbols are allowed to have an empty name, when the constructor of the base class Symbol is executed

[j0le]

lld/COFF/Writer.cpp

399

some questions:

• Should we give the symbol a name to fix the bug, instead of changing Symbol::computeName()?
• Is it allowed, that multiple symbols have the same name?

some questions not really related to the bug, but answers would help me understand LLD better.

• why don't we use SymbolTable::addSynthetic()?
• Do we need to have this symbol in the symbol table?
• Is it added later to the symbol table?

[mstorsjo]

lld/COFF/Symbols.cpp

60 ↗

(On Diff #457568)

This fix kind of changes the assumptions here; the original assumption (see the comment in the assert above in line 57) is that this only ever gets called for DefinedCOFF symbols.

lld/COFF/Symbols.h

85

I'm pretty sure that we can be sure that nameSize is zero here; see line 112 below. (And if we strictly want to keep this, I think we should arrange it like this:

if (nameData == nullptr) {
  computeName();
  if (if nameData == nullptr)
    return StringRef{};
}

But I think we can leave this out entirely.

lld/COFF/Writer.cpp

399

I think just adding a symbol name to fix the bug is better - since apparently the assumption is that only DefinedCOFF symbols may have a non-empty symbol name.

Yes, it's generally allowed that multiple symbols have the same name (consider static symbols within object files).

We don't use SymbolTable::addSynthetic because we don't want to add these symbols into the symbol table (where the name clash with multiple symbols with the same name would be an issue). The exact mechanics of this is a bit of a hazy memory at the moment though, since it's been a few years since I wrote it, and I believe the code might have been restructured at least somewhat since.

lld/test/COFF/arm-thumb-thunks.s

3

I think it might be good to add the same to one of the aarch64 range extension thunk testcases too. From reading the code, it's fairly evident that the same fix automatically applies to both, but it'd be nice to have some test coverage for it too.

[j0le]

lld/COFF/Symbols.cpp

60 ↗

(On Diff #457568)

Should we instead put an assertion in the constructor of DefinedSynthetic, that the name is not allowed to be empty?
Like this:

explicit DefinedSynthetic(StringRef name, Chunk *c)
    : Defined(DefinedSyntheticKind, name), c(c) {
  assert(!name.empty());
}

Or maybe even better in the constructor of Symbol?:

explicit Symbol(Kind k, StringRef n = "")
    : symbolKind(k), isExternal(true), isCOMDAT(false),
      writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false),
      isRuntimePseudoReloc(false), deferUndefined(false), canInline(true),
      nameSize(n.size()), nameData(n.empty() ? nullptr : n.data()) {
  assert((!n.empty() || k <= LastDefinedCOFFKind) &&
         "If the name is empty, the Symbol must be a DefinedCOFF.");
}

[mstorsjo]

lld/COFF/Symbols.cpp

60 ↗

(On Diff #457568)

Yes, I think that might make sense (the latter probably is better), if this is the design of the Symbol classes. But I'd like to hear @rnk's opinion on it too (feel free to update the patch, but I'll at least hold off of landing it until @rnk has commented on it).

[j0le]

Update patch:

• Revert changes, which were made to Symbol::getName() and Symbol::computeName()
• Add assertion to ctor of class Symbol
• Add "-map" to command line of lld-link in test for ARM64

I can confirm, that the bug also occurs for ARM64

[j0le]

Setting phabricator inline comments to "Done".

[mstorsjo]

Thanks, this form looks good to me. I'm still leaving the review open for now, to wait for @rnk's opinion on the direction though.

[j0le]

Thanks everyone for review and feedback! Can someone commit this for me, because I don't have commit access?

[mstorsjo]

In D133201#3773726, @j0le wrote:

Thanks everyone for review and feedback! Can someone commit this for me, because I don't have commit access?

Sure. What's your preferred git author line (Real Name <email@address>) to use for the commit? (I think I'll reword the commit message a little before pushing too.)

[j0le]

This comment was removed by j0le.

[j0le]

In D133201#3773752, @mstorsjo wrote:

I think I'll reword the commit message a little before pushing too.

Yes, that would be nice. I'm not the best writer 😉.

(I think, I will delete the comment with my e-mail address after you have committed the revision, because of spam.)

lld/COFF/Symbols.h

StringRef getName() {
     // symbol index. And because they are not external, no one can refer them by
     // name. Object files contain lots of non-external symbols, and creating
     // StringRefs for them (which involves lots of strlen() on the string table)
     // is a waste of time.
     if (nameData == nullptr)
       computeName();
     return StringRef(nameData, nameSize);
   }
 
   void replaceKeepingName(Symbol *other, size_t size);
 
   // Returns the file from which this symbol was created.
   InputFile *getFile();
 
   // Indicates that this symbol will be included in the final image. Only valid
   // after calling markLive.
   bool isLive() const;
 
   bool isLazy() const {
     return symbolKind == LazyArchiveKind || symbolKind == LazyObjectKind ||
            symbolKind == LazyDLLSymbolKind;
   }
 
 private:
   void computeName();
 
 protected:
   friend SymbolTable;
   explicit Symbol(Kind k, StringRef n = "")
       : symbolKind(k), isExternal(true), isCOMDAT(false),
         writtenToSymtab(false), pendingArchiveLoad(false), isGCRoot(false),
         isRuntimePseudoReloc(false), deferUndefined(false), canInline(true),
-        nameSize(n.size()), nameData(n.empty() ? nullptr : n.data()) {}
+        nameSize(n.size()), nameData(n.empty() ? nullptr : n.data()) {
+    assert((!n.empty() || k <= LastDefinedCOFFKind) &&
+           "If the name is empty, the Symbol must be a DefinedCOFF.");
+  }
 
   const unsigned symbolKind : 8;
   unsigned isExternal : 1;
 
 public:
   // This bit is used by the \c DefinedRegular subclass.
   unsigned isCOMDAT : 1;
 

lld/COFF/Writer.cpp

case ARMNT:
     c = make<RangeExtensionThunkARM>(target);
     break;
   case ARM64:
     c = make<RangeExtensionThunkARM64>(target);
     break;
   default:
     llvm_unreachable("Unexpected architecture");
   }
-  Defined *d = make<DefinedSynthetic>("", c);
+  Defined *d = make<DefinedSynthetic>("range_extension_thunk", c);
   lastThunk = d;
   return {d, true};
 }
 
 // This checks all relocations, and for any relocation which isn't in range
 // it adds a thunk after the section chunk that contains the relocation.
 // If the latest thunk for the specific target is in range, that is used
 // instead of creating a new thunk. All range checks are done with the

lld/test/COFF/arm-thumb-thunks.s

 // REQUIRES: arm
 // RUN: llvm-mc -filetype=obj -triple=thumbv7-windows %s -o %t.obj
-// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
+// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -map -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
 // RUN: llvm-objdump -d %t.exe --start-address=0x401000 --stop-address=0x401022 | FileCheck --check-prefix=MAIN %s
 // RUN: llvm-objdump -d %t.exe --start-address=0x501022 --stop-address=0x501032 | FileCheck --check-prefix=FUNC1 %s
 // RUN: llvm-objdump -d %t.exe --start-address=0x601032 | FileCheck --check-prefix=FUNC2 %s
 
 // VERBOSE: Added 3 thunks with margin {{.*}} in 1 passes
 
     .syntax unified
     .globl main

lld/test/COFF/arm64-thunks.s

 // REQUIRES: aarch64
 // RUN: llvm-mc -filetype=obj -triple=aarch64-windows %s -o %t.obj
-// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
+// RUN: lld-link -entry:main -subsystem:console %t.obj -out:%t.exe -map -verbose 2>&1 | FileCheck -check-prefix=VERBOSE %s
 // RUN: llvm-objdump -d %t.exe | FileCheck --check-prefix=DISASM %s
 
 // VERBOSE: Added 2 thunks with margin {{.*}} in 1 passes
 
     .globl main
     .globl func1
     .globl func2
     .text