COFF: Fix a common symbol bug.

This is a case that one mistake caused a very mysterious bug.
I made a mistake to calculate addresses of common symbols, so
each common symbol pointed not to the beginning of its location
but to the end of its location. (Ouch!)

Common symbols are aligned on 16 byte boundaries. If a common
symbol is small enough to fit between the end of its real
location and whatever comes next, this bug didn't cause any harm.

However, if a common symbol is larger than that, its memory
naturally overlapped with other symbols. That means some
uninitialized variables accidentally shared memory. Because
totally unrelated memory writes mutated other varaibles, it was
hard to debug.

It's surprising that LLD was able to link itself and all LLD
tests except gunit tests passed with this nasty bug.

With this fix, the new COFF linker is able to pass all tests
for LLVM, Clang and LLD if I use MSVC cl.exe as a compiler.
Only three tests are failing when used with clang-cl.

llvm-svn: 240216

lld/COFF/InputFiles.cpp

@@ -184,7 +184,7 @@ SymbolBody *ObjectFile::createSymbolBody(COFFSymbolRef Sym, const void *AuxP,
   if (Sym.isCommon()) {
     Chunk *C = new (Alloc) CommonChunk(Sym);
     Chunks.push_back(C);
-    return new (Alloc) DefinedRegular(COFFObj.get(), Sym, C);
+    return new (Alloc) DefinedCommon(COFFObj.get(), Sym, C);
   }
   if (Sym.isAbsolute()) {
     COFFObj->getSymbolName(Sym, Name);

lld/COFF/Symbols.cpp

@@ -29,21 +29,21 @@ int DefinedRegular::compare(SymbolBody *Other) {
   auto *R = dyn_cast<DefinedRegular>(Other);
   if (!R)
     return 1;
-
-  // Common symbols are weaker than other types of defined symbols.
-  if (isCommon() && R->isCommon())
-    return (getCommonSize() < R->getCommonSize()) ? -1 : 1;
-  // TODO: we are not sure if regular defined symbol and common
-  // symbols are allowed to have the same name.
-  if (isCommon())
-    return -1;
-  if (R->isCommon())
-    return 1;
   if (isCOMDAT() && R->isCOMDAT())
     return 1;
   return 0;
 }
 
+int DefinedCommon::compare(SymbolBody *Other) {
+  if (Other->kind() < kind())
+    return -Other->compare(this);
+  if (auto *D = dyn_cast<DefinedCommon>(Other))
+    return getSize() > D->getSize() ? 1 : -1;
+  if (isa<DefinedRegular>(Other))
+    return -1;
+  return 1;
+}
+
 int DefinedBitcode::compare(SymbolBody *Other) {
   assert(Other->kind() >= kind());
   if (!isa<Defined>(Other))
@@ -63,10 +63,12 @@ int DefinedBitcode::compare(SymbolBody *Other) {
   // resolution will be done accurately after lowering bitcode symbols
   // to regular symbols in addCombinedLTOObject().
   if (auto *R = dyn_cast<DefinedRegular>(Other)) {
-    if (!R->isCommon() && !R->isCOMDAT() && !Replaceable)
+    if (!R->isCOMDAT() && !Replaceable)
       return 0;
     return -1;
   }
+  if (isa<DefinedCommon>(Other))
+    return -1;
   return 0;
 }
 
@@ -113,6 +115,12 @@ StringRef DefinedRegular::getName() {
   return Name;
 }
 
+StringRef DefinedCommon::getName() {
+  if (Name.empty())
+    COFFFile->getSymbolName(Sym, Name);
+  return Name;
+}
+
 ErrorOr<std::unique_ptr<InputFile>> Lazy::getMember() {
   auto MBRefOrErr = File->getMember(&Sym);
   if (auto EC = MBRefOrErr.getError())

lld/COFF/Symbols.h

@@ -49,6 +49,7 @@ public:
     DefinedAbsoluteKind,
     DefinedImportDataKind,
     DefinedImportThunkKind,
+    DefinedCommonKind,
     DefinedRegularKind,
     DefinedLast,
     LazyKind,
@@ -130,11 +131,32 @@ public:
   bool isCOMDAT() const { return Data->isCOMDAT(); }
   int compare(SymbolBody *Other) override;
 
-  // Returns true if this is a common symbol.
-  bool isCommon() const { return Sym.isCommon(); }
-  uint32_t getCommonSize() const { return Sym.getValue(); }
+private:
+  StringRef Name;
+  COFFObjectFile *COFFFile;
+  COFFSymbolRef Sym;
+  Chunk *Data;
+};
+
+class DefinedCommon : public Defined {
+public:
+  DefinedCommon(COFFObjectFile *F, COFFSymbolRef S, Chunk *C)
+      : Defined(DefinedCommonKind), COFFFile(F), Sym(S), Data(C) {}
+
+  static bool classof(const SymbolBody *S) {
+    return S->kind() == DefinedCommonKind;
+  }
+
+  StringRef getName() override;
+  uint64_t getRVA() override { return Data->getRVA(); }
+  bool isExternal() override { return Sym.isExternal(); }
+  void markLive() override { Data->markLive(); }
+  uint64_t getFileOff() override { return Data->getFileOff(); }
+  int compare(SymbolBody *Other) override;
 
 private:
+  uint64_t getSize() { return Sym.getValue(); }
+
   StringRef Name;
   COFFObjectFile *COFFFile;
   COFFSymbolRef Sym;

lld/test/COFF/Inputs/common.yaml

@@ -0,0 +1,91 @@
+---
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: []
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     b800000000b800000000b800000000b800000000b800000000
+    Relocations:
+      - VirtualAddress:  1
+        SymbolName:      bssdata4
+        Type:            IMAGE_REL_AMD64_ADDR32
+      - VirtualAddress:  6
+        SymbolName:      bsspad1
+        Type:            IMAGE_REL_AMD64_ADDR32
+      - VirtualAddress:  11
+        SymbolName:      bssdata64
+        Type:            IMAGE_REL_AMD64_ADDR32
+      - VirtualAddress:  16
+        SymbolName:      bsspad2
+        Type:            IMAGE_REL_AMD64_ADDR32
+      - VirtualAddress:  21
+        SymbolName:      bssdata16
+        Type:            IMAGE_REL_AMD64_ADDR32
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     03000000
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 5
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            mainCRTStartup
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssdata4
+    Value:           4
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bsspad1
+    Value:           1
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssdata64
+    Value:           64
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bsspad2
+    Value:           1
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssdata16
+    Value:           16
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

lld/test/COFF/common.test

@@ -0,0 +1,10 @@
+# RUN: yaml2obj %p/Inputs/common.yaml > %t.obj
+# RUN: lld -flavor link2 /out:%t.exe %t.obj %t.obj
+# RUN: llvm-objdump -d %t.exe | FileCheck %s
+
+# Operands of B8 (MOV EAX) are common symbols
+CHECK: 3000: b8 00 10 00 40
+CHECK: 3005: b8 10 10 00 40
+CHECK: 300a: b8 20 10 00 40
+CHECK: 300f: b8 60 10 00 40
+CHECK: 3014: b8 70 10 00 40