[LLD] [COFF] Don't error out on duplicate absolute symbols with the same value

[mstorsjo]

Summary

Both MS link.exe and GNU ld.bfd handle it this way; one can have multiple object files defining the same absolute symbols, as long as it defines it to the same value. But if there are multiple absolute symbols with differing values, it is treated as an error.

As a general related note - LLD differs from link.exe (and GNU ld.bfd) when it comes to conflicts between a defined absolute regular and defined regular; LLD silently accepts these (in both orders), while both link.exe and ld.bfd error out on it.

[rnk]

lld/COFF/SymbolTable.cpp

597

This looks like the code that makes LLD not report duplicates between absolute and regular symbols. Should we re-evaluate that decision?

The logic dates back to 79a5e6b1b77efe7770503ebce2a808f0b89d9e50, which doesn't explicitly mention this case. That makes me think this behavior is not intentional, and we can change it.

[mstorsjo]

lld/COFF/SymbolTable.cpp

597

Yes, we could change it, but I'd treat it as a different change from this one.

As things stand right now, leaving D71711 should work fine, but if we make absolute+regular an error, we'd break the case with multiple objects referencing the same weak, if one object provides a default function which isn't absolute zero.

[rnk]

lgtm

[ruiu]

lld/COFF/Symbols.h

232–238

Instead of adding these member functions, maybe we should just add an accessor getVA? It looks like there's no point to hide the actual VA from other classes.

[MaskRay]

lld/test/COFF/duplicate-absolute-same.s

5

If there is no diagnostic, use count 0. Then, CHECK-NOT can be turned into a comment.

Negative diagnostics checks may become stale when the message changes.

[mstorsjo]

lld/COFF/Symbols.h

232–238

Sure, I can do that.

lld/test/COFF/duplicate-absolute-same.s

5

Ok, will do.

Yes, there's a clear risk with checking for the error message contents - but if really was an error, it would still be picked up as the lld-link command also would return a failure status. But worth fixing anyway.

lld/COFF/SymbolTable.cpp

 
 Symbol *SymbolTable::addAbsolute(StringRef n, COFFSymbolRef sym) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(n, nullptr);
   s->isUsedInRegularObj = true;
   if (wasInserted || isa<Undefined>(s) || s->isLazy())
     replaceSymbol<DefinedAbsolute>(s, n, sym);
-  else if (!isa<DefinedCOFF>(s))
+  else if (auto *da = dyn_cast<DefinedAbsolute>(s)) {
+    if (!da->isEqual(sym))
+      reportDuplicate(s, nullptr);
+  } else if (!isa<DefinedCOFF>(s))
     reportDuplicate(s, nullptr);
   return s;
 }
 
 Symbol *SymbolTable::addAbsolute(StringRef n, uint64_t va) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(n, nullptr);
   s->isUsedInRegularObj = true;
   if (wasInserted || isa<Undefined>(s) || s->isLazy())
     replaceSymbol<DefinedAbsolute>(s, n, va);
-  else if (!isa<DefinedCOFF>(s))
+  else if (auto *da = dyn_cast<DefinedAbsolute>(s)) {
+    if (!da->isEqual(va))
+      reportDuplicate(s, nullptr);
+  } else if (!isa<DefinedCOFF>(s))
     reportDuplicate(s, nullptr);
   return s;
 }
 
 Symbol *SymbolTable::addSynthetic(StringRef n, Chunk *c) {
   Symbol *s;
   bool wasInserted;
   std::tie(s, wasInserted) = insert(n, nullptr);

lld/COFF/Symbols.h

public:
 
   static bool classof(const Symbol *s) {
     return s->kind() == DefinedAbsoluteKind;
   }
 
   uint64_t getRVA() { return va - config->imageBase; }
   void setVA(uint64_t v) { va = v; }
 
+  bool isEqual(COFFSymbolRef s) const {
+    return va == s.getValue();
+  }
+
+  bool isEqual(uint64_t otherVa) const {
+    return va == otherVa;
+  }
+
   // Section index relocations against absolute symbols resolve to
   // this 16 bit number, and it is the largest valid section index
   // plus one. This variable keeps it.
   static uint16_t numOutputSections;
 
 private:
   uint64_t va;
 };

lld/test/COFF/duplicate-absolute-same.s

 // REQUIRES: x86
 // RUN: llvm-mc -triple x86_64-windows-msvc -filetype obj -o %t.obj %s
 // RUN: echo -e ".globl myabsolute\nmyabsolute = 0" > %t.dupl.s
 // RUN: llvm-mc -triple x86_64-windows-msvc -filetype obj -o %t.dupl.obj %t.dupl.s
-// RUN: not lld-link /out:%t.exe %t.obj %t.dupl.obj 2>&1 | FileCheck %s
+// RUN: lld-link /out:%t.exe %t.obj %t.dupl.obj -subsystem:console -entry:entry 2>&1 | FileCheck --allow-empty %s
 
-// CHECK: error: duplicate symbol: myabsolute
+// CHECK-NOT: error: duplicate symbol: myabsolute
 
 .globl myabsolute
 myabsolute = 0
 
 .globl entry
 entry:
     ret

lld/test/COFF/duplicate-absolute.s

 // REQUIRES: x86
 // RUN: llvm-mc -triple x86_64-windows-msvc -filetype obj -o %t.obj %s
-// RUN: echo -e ".globl myabsolute\nmyabsolute = 0" > %t.dupl.s
+// RUN: echo -e ".globl myabsolute\nmyabsolute = 1" > %t.dupl.s
 // RUN: llvm-mc -triple x86_64-windows-msvc -filetype obj -o %t.dupl.obj %t.dupl.s
 // RUN: not lld-link /out:%t.exe %t.obj %t.dupl.obj 2>&1 | FileCheck %s
 
 // CHECK: error: duplicate symbol: myabsolute
 
 .globl myabsolute
 myabsolute = 0
 
 .globl entry
 entry:
     ret