[COFF] Assign unique names to autogenerated .weak.<name>.default symbols

[mstorsjo]

Summary

These symbols need to be external (MSVC tools error out if a weak external points at a symbol that isn't external; this was tried before but had to be reverted in bc5b7217dceecd3eec69593026a9e38dfbfd6908, and this was originally explicitly fixed in 732eeaf2a930ad2755cb4eb5d99a3deae0de4a72).

If multiple object files have weak symbols with defaults, their defaults could cause linker errors due to duplicate definitions, unless the names of the defaults are unique.

GNU binutils handles this by appending the name of another symbol from the same object file to the name of the default symbol. Try to implement something similar; before writing the object file, locate a symbol that should have a unique name and use the name of that one for making the weak defaults unique.

This supersedes D71711.

[mstorsjo]

Fixed a clang-tidy warning.

[rnk]

lgtm

This is pretty interesting behavior. :)

llvm/lib/MC/WinCOFFObjectWriter.cpp

 //===- llvm/MC/WinCOFFObjectWriter.cpp ------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains an implementation of a Win32 COFF object file writer.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/MC/MCAsmLayout.h"
 #include "llvm/MC/MCAssembler.h"
public:
   support::endian::Writer W;
 
   using symbols = std::vector<std::unique_ptr<COFFSymbol>>;
   using sections = std::vector<std::unique_ptr<COFFSection>>;
 
   using symbol_map = DenseMap<MCSymbol const *, COFFSymbol *>;
   using section_map = DenseMap<MCSection const *, COFFSection *>;
 
+  using symbol_list = DenseSet<COFFSymbol *>;
+
   std::unique_ptr<MCWinCOFFObjectTargetWriter> TargetObjectWriter;
 
   // Root level file contents.
   COFF::header Header = {};
   sections Sections;
   symbols Symbols;
   StringTableBuilder Strings{StringTableBuilder::WinCOFF};
 
   // Maps used during object file creation.
   section_map SectionMap;
   symbol_map SymbolMap;
 
+  symbol_list WeakDefaults;
+
   bool UseBigObj;
 
   bool EmitAddrsigSection = false;
   MCSectionCOFF *AddrsigSection;
   std::vector<const MCSymbol *> AddrsigSyms;
 
   WinCOFFObjectWriter(std::unique_ptr<MCWinCOFFObjectTargetWriter> MOTW,
                       raw_pwrite_stream &OS);
bool isSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
                                               const MCFragment &FB, bool InSet,
                                               bool IsPCRel) const override;
 
   void recordRelocation(MCAssembler &Asm, const MCAsmLayout &Layout,
                         const MCFragment *Fragment, const MCFixup &Fixup,
                         MCValue Target, uint64_t &FixedValue) override;
 
   void createFileSymbols(MCAssembler &Asm);
+  void setWeakDefaultNames();
   void assignSectionNumbers();
   void assignFileOffsets(MCAssembler &Asm, const MCAsmLayout &Layout);
 
   void emitAddrsigSection() override { EmitAddrsigSection = true; }
   void addAddrsigSymbol(const MCSymbol *Sym) override {
     AddrsigSyms.push_back(Sym);
   }
 
if (cast<MCSymbolCOFF>(MCSym).isWeakExternal()) {
     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;
+      WeakDefaults.insert(WeakDefault);
       Local = WeakDefault;
     }
 
     Sym->Other = WeakDefault;
 
     // Setup the Weak External auxiliary symbol.
     Sym->Aux.resize(1);
     memset(&Sym->Aux[0], 0, sizeof(Sym->Aux[0]));
for (auto &Aux : File->Aux) {
         break;
       }
 
       Offset += SymbolSize;
     }
   }
 }
 
+void WinCOFFObjectWriter::setWeakDefaultNames() {
+  if (WeakDefaults.empty())
+    return;
+
+  // If multiple object files use a weak symbol (either with a regular
+  // defined default, or an absolute zero symbol as default), the defaults
+  // cause duplicate definitions unless their names are made unique. Look
+  // for a defined extern symbol, that isn't comdat - that should be unique
+  // unless there are other duplicate definitions. And if none is found,
+  // allow picking a comdat symbol, as that's still better than nothing.
+
+  COFFSymbol *Unique = nullptr;
+  for (bool AllowComdat : {false, true}) {
+    for (auto &Sym : Symbols) {
+      // Don't include the names of the defaults themselves
+      if (WeakDefaults.count(Sym.get()))
+        continue;
+      // Only consider external symbols
+      if (Sym->Data.StorageClass != COFF::IMAGE_SYM_CLASS_EXTERNAL)
+        continue;
+      // Only consider symbols defined in a section or that are absolute
+      if (!Sym->Section && Sym->Data.SectionNumber != COFF::IMAGE_SYM_ABSOLUTE)
+        continue;
+      if (!AllowComdat && Sym->Section &&
+          Sym->Section->Header.Characteristics & COFF::IMAGE_SCN_LNK_COMDAT)
+        continue;
+      Unique = Sym.get();
+      break;
+    }
+    if (Unique)
+      break;
+  }
+  // If we didn't find any unique symbol to use for the names, just skip this.
+  if (!Unique)
+    return;
+  for (auto *Sym : WeakDefaults) {
+    Sym->Name.append(".");
+    Sym->Name.append(Unique->Name);
+  }
+}
+
 static bool isAssociative(const COFFSection &Section) {
   return Section.Symbol->Aux[0].Aux.SectionDefinition.Selection ==
          COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE;
 }
 
 void WinCOFFObjectWriter::assignSectionNumbers() {
   size_t I = 1;
   auto Assign = [&](COFFSection &Section) {
uint64_t WinCOFFObjectWriter::writeObject(MCAssembler &Asm,
   if (Sections.size() > INT32_MAX)
     report_fatal_error(
         "PE COFF object files can't have more than 2147483647 sections");
 
   UseBigObj = Sections.size() > COFF::MaxNumberOfSections16;
   Header.NumberOfSections = Sections.size();
   Header.NumberOfSymbols = 0;
 
+  setWeakDefaultNames();
   assignSectionNumbers();
   createFileSymbols(Asm);
 
   for (auto &Symbol : Symbols) {
     // Update section number & offset for symbols that have them.
     if (Symbol->Section)
       Symbol->Data.SectionNumber = Symbol->Section->Number;
     Symbol->setIndex(Header.NumberOfSymbols++);

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

 // CHECK-NEXT:   StorageClass: WeakExternal (0x69)
 // CHECK-NEXT:   AuxSymbolCount: 1
 // CHECK-NEXT:   AuxWeakExternal {
 // CHECK-NEXT:     Linked: .weak.a.default (9)
 // CHECK-NEXT:     Search: Alias (0x3)
 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 // CHECK-NEXT: Symbol {
-// CHECK-NEXT:   Name: .weak.a.default
+// 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:   AuxSymbolCount: 0
 // CHECK-NEXT: }

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

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s -defsym case=0 -o %t.case0.o
+// RUN: llvm-readobj --symbols %t.case0.o | FileCheck %s --check-prefix=CHECK-CASE0
+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s -defsym case=1 -o %t.case1.o
+// RUN: llvm-readobj --symbols %t.case1.o | FileCheck %s --check-prefix=CHECK-CASE1
+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-win32 %s -defsym case=2 -o %t.case2.o
+// RUN: llvm-readobj --symbols %t.case2.o | FileCheck %s --check-prefix=CHECK-CASE2
+
+// Test that we prefer a non-comdat symbol for naming weak default symbols,
+// if such a symbol is available.
+
+        .section .text$comdat1,"xr",discard,comdat1
+        .globl   comdat1
+comdat1:
+        call     undeffunc
+
+        .weak    weaksym
+
+        .section .text$comdat2,"xr",discard,comdat2
+        .globl   comdat2
+comdat2:
+        call     undeffunc2
+
+.if case == 0
+        .text
+        .globl   regular
+regular:
+        call     undeffunc3
+.elseif case == 1
+        .globl   abssym
+abssym = 42
+.endif
+
+// CHECK-CASE0: Name: .weak.weaksym.default.regular
+// CHECK-CASE1: Name: .weak.weaksym.default.abssym
+// CHECK-CASE2: Name: .weak.weaksym.default.comdat1

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

 // CHECK-NEXT:   StorageClass: WeakExternal (0x69)
 // CHECK-NEXT:   AuxSymbolCount: 1
 // CHECK-NEXT:   AuxWeakExternal {
 // CHECK-NEXT:     Linked: .weak.b.default (8)
 // CHECK-NEXT:     Search: Alias (0x3)
 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 // CHECK-NEXT: Symbol {
-// CHECK-NEXT:   Name: .weak.b.default
+// 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:   AuxSymbolCount: 0
 // CHECK-NEXT: }

llvm/test/MC/COFF/weak.s

 // CHECK-NEXT:   AuxSymbolCount: 1
 // CHECK-NEXT:   AuxWeakExternal {
 // CHECK-NEXT:     Linked: .weak._test_weak.default
 // CHECK-NEXT:      Search: Alias
 // CHECK-NEXT:   }
 // CHECK-NEXT: }
 
 // CHECK:      Symbol {
-// CHECK:        Name:                .weak._test_weak.default
+// CHECK:        Name:                .weak._test_weak.default._main
 // 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:   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:        Name: .weak._test_weak_alias.default._main
 // CHECK-NEXT:   Value: 0
 // CHECK-NEXT:   Section: .text
 // CHECK-NEXT:   BaseType: Null
 // CHECK-NEXT:   ComplexType: Null
 // CHECK-NEXT:   StorageClass: External
 // CHECK-NEXT:   AuxSymbolCount: 0
 // CHECK-NEXT: }