[COFF] Make the autogenerated .weak.<name>.default symbols static

[mstorsjo]

Summary

If we have references to the same extern_weak in multiple objects, all of them would generate external symbols with the same name. Make them static to avoid duplicate definitions; nothing should need to refer to this symbol outside of the current object.

GCC/binutils seems to handle the same by not using a fixed string for the ".default" suffix, but instead using the name of some other defined external symbol from the same object (which is supposed to be unique among objects unless there are other duplicate definitions).

[mstorsjo]

Ping @rnk

[rnk]

llvm/lib/MC/WinCOFFObjectWriter.cpp

369

While here, I think it would be better to hoist the cast into the entry block, rename the parameter to MCSymGeneric, and have a new local variable const auto &MCSym = cast<MCSymbolCOFF>(MCSymGeneric);

383

I think we can assign static storage here.

400

So, we could move the old storage class calculation here.

415

Can this be factored so we only check for weak externals once? We do it above and assign it to Local. All this StorageClass code is dead for the weak external case, so we can hoist this new StorageClass up to that block, and hoist the old StorageClass code into the else block.

[mstorsjo]

Did as suggested. As some of the moved blocks are larger than what was left unmoved, parts of the diff is a bit nonobvious though.

[rnk]

lgtm, thanks!

[mstorsjo]

Ok, so apparently this doesn't work, as MS link.exe rejects object files, where a IMAGE_SYM_CLASS_WEAK_EXTERNAL symbol points at a symbol which isn't external - so I had to revert it.

Are there any other options then, than trying to do what GCC/binutils does, by changing the suffix .default into something that more hopefully is unique? They use the name of another defined (probably non-comdat) external symbol from the same object (that should be unique) as suffix. But that feels like a much larger mess to implement.

FWIW, right now this is blocking me from llvm with clang itself for mingw targets, since we started using an external thread_local variable.

[mstorsjo]

In D71711#1798098, @mstorsjo wrote:

Are there any other options then, than trying to do what GCC/binutils does, by changing the suffix .default into something that more hopefully is unique? They use the name of another defined (probably non-comdat) external symbol from the same object (that should be unique) as suffix. But that feels like a much larger mess to implement.

FWIW, right now this is blocking me from llvm with clang itself for mingw targets, since we started using an external thread_local variable.

Making lld more lenient with multiple object files with the same default absolute symbol for the address zero should fix the primary issue wrt the TLS initializer function (although I haven't tried rebuilding everything with that in place), which I implemented in D71981. Implementing something similar to binutils with making unique names for the weak default symbols would still be nice though.

[rnk]

I guess if the absolute symbol change is enough to unblock you, I'd leave this alone for now. The gnu behavior seems error prone and difficult to implement.

[mstorsjo]

In D71711#1802223, @rnk wrote:

I guess if the absolute symbol change is enough to unblock you, I'd leave this alone for now. The gnu behavior seems error prone and difficult to implement.

Yeah, it seems hard to do sensibly - but it'd be necessary (in the generic case, not for the present weak tls init function case) if the linker errors out on combinations of absolute zero and regular. So strictly, it'd be necessary for clang+ld.bfd, for a wider use of weak, and also for lld if we change that aspect there.

[mstorsjo]

Superseded by D75989.

llvm/lib/MC/WinCOFFObjectWriter.cpp

COFFSymbol *WinCOFFObjectWriter::getLinkedSymbol(const MCSymbol &Symbol) {
   const MCSymbol &Aliasee = SymRef->getSymbol();
   if (!Aliasee.isUndefined())
     return nullptr;
   return GetOrCreateCOFFSymbol(&Aliasee);
 }
 
 /// This function takes a symbol data object from the assembler
 /// and creates the associated COFF symbol staging object.
-void WinCOFFObjectWriter::DefineSymbol(const MCSymbol &MCSym,
+void WinCOFFObjectWriter::DefineSymbol(const MCSymbol &MCSymGeneric,
                                        MCAssembler &Assembler,
                                        const MCAsmLayout &Layout) {
+  const auto &MCSym = cast<MCSymbolCOFF>(MCSymGeneric);
   COFFSymbol *Sym = GetOrCreateCOFFSymbol(&MCSym);
   const MCSymbol *Base = Layout.getBaseSymbol(MCSym);
   COFFSection *Sec = nullptr;
   if (Base && Base->getFragment()) {
     Sec = SectionMap[Base->getFragment()->getParent()];
     if (Sym->Section && Sym->Section != Sec)
       report_fatal_error("conflicting sections for symbol");
   }
 
   COFFSymbol *Local = nullptr;
-  if (cast<MCSymbolCOFF>(MCSym).isWeakExternal()) {
+  if (MCSym.isWeakExternal()) {
     Sym->Data.StorageClass = COFF::IMAGE_SYM_CLASS_WEAK_EXTERNAL;
 
     COFFSymbol *WeakDefault = getLinkedSymbol(MCSym);
     if (!WeakDefault) {
       std::string WeakName = (".weak." + MCSym.getName() + ".default").str();
       WeakDefault = createSymbol(WeakName);
       if (!Sec)
         WeakDefault->Data.SectionNumber = COFF::IMAGE_SYM_ABSOLUTE;
       else
         WeakDefault->Section = Sec;
+      // Make the default symbol static, in order to not conflict with
+      // similar default symbols for the same weak in other objects.
+      WeakDefault->Data.StorageClass = COFF::IMAGE_SYM_CLASS_STATIC;
       Local = WeakDefault;
     }
 
     Sym->Other = WeakDefault;
 
     // Setup the Weak External auxiliary symbol.
     Sym->Aux.resize(1);
     memset(&Sym->Aux[0], 0, sizeof(Sym->Aux[0]));
     Sym->Aux[0].AuxType = ATWeakExternal;
     Sym->Aux[0].Aux.WeakExternal.TagIndex = 0;
     Sym->Aux[0].Aux.WeakExternal.Characteristics =
         COFF::IMAGE_WEAK_EXTERN_SEARCH_ALIAS;
   } else {
     if (!Base)
       Sym->Data.SectionNumber = COFF::IMAGE_SYM_ABSOLUTE;
     else
       Sym->Section = Sec;
     Local = Sym;
-  }
 
-  if (Local) {
-    Local->Data.Value = getSymbolValue(MCSym, Layout);
-
-    const MCSymbolCOFF &SymbolCOFF = cast<MCSymbolCOFF>(MCSym);
-    Local->Data.Type = SymbolCOFF.getType();
-    Local->Data.StorageClass = SymbolCOFF.getClass();
+    Local->Data.StorageClass = MCSym.getClass();
 
     // If no storage class was specified in the streamer, define it here.
     if (Local->Data.StorageClass == COFF::IMAGE_SYM_CLASS_NULL) {
       bool IsExternal = MCSym.isExternal() ||
                         (!MCSym.getFragment() && !MCSym.isVariable());
 
       Local->Data.StorageClass = IsExternal ? COFF::IMAGE_SYM_CLASS_EXTERNAL
                                             : COFF::IMAGE_SYM_CLASS_STATIC;
     }
   }
 
+  if (Local) {
+    Local->Data.Value = getSymbolValue(MCSym, Layout);
+
+    Local->Data.Type = MCSym.getType();
+  }
+
   Sym->MC = &MCSym;
 }
 
 // Maximum offsets for different string table entry encodings.
 enum : unsigned { Max7DecimalOffset = 9999999U };
 enum : uint64_t { MaxBase64Offset = 0xFFFFFFFFFULL }; // 64^6, including 0
 
 // Encode a string table entry offset in base 64, padded to 6 chars, and

llvm/test/MC/COFF/weak-alias-local.s

 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 // CHECK-NEXT: Symbol {
 // CHECK-NEXT:   Name: .weak.a.default
 // CHECK-NEXT:   Value: 4
 // CHECK-NEXT:   Section: .data (2)
 // CHECK-NEXT:   BaseType: Null (0x0)
 // CHECK-NEXT:   ComplexType: Null (0x0)
-// CHECK-NEXT:   StorageClass: External (0x2)
+// CHECK-NEXT:   StorageClass: Static (0x3)
 // CHECK-NEXT:   AuxSymbolCount: 0
 // CHECK-NEXT: }

llvm/test/MC/COFF/weak-val.s

 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 // CHECK-NEXT: Symbol {
 // CHECK-NEXT:   Name: .weak.b.default
 // CHECK-NEXT:   Value: 4
 // CHECK-NEXT:   Section: .data (2)
 // CHECK-NEXT:   BaseType: Null (0x0)
 // CHECK-NEXT:   ComplexType: Null (0x0)
-// CHECK-NEXT:   StorageClass: External (0x2)
+// CHECK-NEXT:   StorageClass: Static (0x3)
 // CHECK-NEXT:   AuxSymbolCount: 0
 // CHECK-NEXT: }

llvm/test/MC/COFF/weak.s

 // CHECK-NEXT: }
 
 // CHECK:      Symbol {
 // CHECK:        Name:                .weak._test_weak.default
 // CHECK-NEXT:   Value:               0
 // CHECK-NEXT:   Section:             IMAGE_SYM_ABSOLUTE (-1)
 // CHECK-NEXT:   BaseType:            Null
 // CHECK-NEXT:   ComplexType:         Null
-// CHECK-NEXT:   StorageClass:        External
+// CHECK-NEXT:   StorageClass:        Static
 // CHECK-NEXT:   AuxSymbolCount:      0
 // CHECK-NEXT: }
 
 // CHECK:      Symbol {
 // CHECK:        Name:           _test_weak_alias
 // CHECK-NEXT:   Value:          0
 // CHECK-NEXT:   Section:        IMAGE_SYM_UNDEFINED (0)
 // CHECK-NEXT:   BaseType:       Null
 // CHECK-NEXT:   ComplexType:    Null
 // CHECK-NEXT:   StorageClass:   WeakExternal
 // CHECK-NEXT:   AuxSymbolCount: 1
 // CHECK-NEXT:   AuxWeakExternal {
 // CHECK-NEXT:     Linked: .weak._test_weak_alias.default
 // CHECK-NEXT:      Search: Alias
 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 
 // CHECK:      Symbol {
 // CHECK:        Name: .weak._test_weak_alias.default
 // CHECK-NEXT:   Value: 0
 // CHECK-NEXT:   Section: .text
 // CHECK-NEXT:   BaseType: Null
 // CHECK-NEXT:   ComplexType: Null
-// CHECK-NEXT:   StorageClass: External
+// CHECK-NEXT:   StorageClass: Static
 // CHECK-NEXT:   AuxSymbolCount: 0
 // CHECK-NEXT: }