[mingw] Fix GCC ABI compatibility for comdat things

[rnk]

Summary

GCC and the binutils COFF linker do comdats differently from MSVC.
If we want to be ABI compatible, we have to do what they do, which is to
emit unique section names like ".text$_Z3foov" instead of short section
names like ".text". Otherwise, the binutils linker gets confused and
reports multiple definition errors when two object files from GCC and
Clang containing the same inline function are linked together.

The best description of the issue is probably at
https://github.com/Alexpux/MINGW-packages/issues/1677, we don't seem to
have a good one in our tracker.

I fixed up the .pdata and .xdata sections needed everywhere other than
32-bit x86. GCC doesn't use associative comdats for those, it appears to
rely on the section name.

[rnk]

• add missing test

[mstorsjo]

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

[rnk]

In D48402#1138925, @mstorsjo wrote:

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

Hm, I would expect LLD to do the wrong thing for .pdata and .xdata under this scheme. Consider this example where I've made an inline function that has some .pdata:

$ cat t.h
void force_pdata();
inline int foo(int a, int b) {
  force_pdata();
  return a + b;
}

$ cat a.cpp
#include "t.h"
void force_pdata() {}
int bar(void);
int __declspec(dllexport) asdf() { return foo(1, 2) + bar(); }

$ cat b.cpp
#include "t.h"
int bar() { return foo(3, 4); }

$ gcc -c a.cpp && gcc -c b.cpp && lld-link a.o b.o -dll -out:t.dll  -noentry

$ dumpbin -disasm -pdata t.dll
... Disassembly shows four function prologues at:
0000000180001000:
0000000180001002:
0000000180001030:
0000000180001050:

Function Table (3)

           Begin    End      Info      Function Name

  00000000 00001000 00001002 000020C8
  0000000C 00001002 0000102F 000020CC
  00000018 00001030 0000104D 000020D8

It looks like LLD is tossing the otherwise unreferenced .pdata and .xdata for the inline function that starts at RVA 0x001050.

If we want LLD to support linking mingw object files from GCC, we'll need to do some extra work in LLD.

[mstorsjo]

In D48402#1139600, @rnk wrote:

In D48402#1138925, @mstorsjo wrote:

Seems sensible to me. I've tested this change with LLD as linker as well, and it worked just fine for building Qt, and for a small test setup similar to the example in the issue report.

Hm, I would expect LLD to do the wrong thing for .pdata and .xdata under this scheme.

Oh - I didn't actually test anything that uses that; I'm only using sjlj or dwarf for exception handling ATM, since libcxxabi doesn't support SEH exeptions yet.

It looks like LLD is tossing the otherwise unreferenced .pdata and .xdata for the inline function that starts at RVA 0x001050.

If we want LLD to support linking mingw object files from GCC, we'll need to do some extra work in LLD.

Even if they're supposed to be interoperable, more or less, it's not completely compatible in all ways.

Currently, there's three different MinGW scenarios wrt compilation+linking that actually work:

1. Compiling with GCC, linking with ld.bfd (a completely traditional MinGW environment, normally together with libgcc and libstdc++)
2. Compiling with clang, linking with ld.bfd (just plugging in clang as compiler in an otherwise traditional MinGW environment, still using libgcc+libstdc++)
3. Compiling with clang, linking with LLD (using compiler-rt and libcxx)

The third setup is what I and Martell have been working on; the produced object files should otherwise be identical to the ones in case 2 (modulo any use of -fdwarf-exceptions). The fourth plausible combo, compiling with GCC but linking with LLD, most probably doesn't work right now - I haven't tried but would expect there to be lots of details missing in LLD.

If we make clang start producing this, how hard would it be to make LLD handle it gracefully? (I don't think I've touched the comdat handling in LLD significantly.)

[rnk]

In D48402#1139787, @mstorsjo wrote:

If we make clang start producing this, how hard would it be to make LLD handle it gracefully? (I don't think I've touched the comdat handling in LLD significantly.)

Probably not too hard. We'd make a special case for comdat .pdata and .xdata (and maybe .ctors and .dtors) with $ in the input section name. We'd take the symbol name after the $, look that symbol up, and pretend that this section was comdat associative with it.

[pirama]

Reid, thanks a lot for addressing this issue.

(For reference to @mstorsjo, I am trying option #2 that you mention).

I fixed up the .pdata and .xdata sections needed everywhere other than
32-bit x86. GCC doesn't use associative comdats for those, it appears to
rely on the section name.

With your change, the duplicate definition errors for x86_64 are addressed but still happen for 32-bit x86.

I also found that for x86_64, the section also has a comdat tag.

Sections:
....
  4 .text$_Z3fooi 00000014  0000000000000000  0000000000000000  000001bc  2**4
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD (COMDAT _Z3fooi 10)

while for i686, it doesn't:

Sections:
....
  3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
                 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

[rnk]

In D48402#1140019, @pirama wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

[pirama]

In D48402#1140046, @rnk wrote:

In D48402#1140019, @pirama wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

I replaced getMangler().getNameWithPrefix(SecName, Symbol, DL); in appendComdatSymbolForMinGW with SecName.append(Symbol.begin(), Symbol.end());. This removes the extra underscore but still doesn't associate a comdat with the section. Is there anything else that I can try?

[smeenai]

In D48402#1141048, @pirama wrote:

In D48402#1140046, @rnk wrote:

In D48402#1140019, @pirama wrote:

Sections:
....

3 .text$___Z3fooi 00000013  00000000  00000000  0000012c  2**4
               CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE, LINK_ONCE_DISCARD

Could that explain the difference?

Woops, one too many calls to getNameWithPrefix.

I replaced getMangler().getNameWithPrefix(SecName, Symbol, DL); in appendComdatSymbolForMinGW with SecName.append(Symbol.begin(), Symbol.end());. This removes the extra underscore but still doesn't associate a comdat with the section. Is there anything else that I can try?

Reid fixed the underscore issue in rL335304, so you may wanna give that a shot.

[rnk]

In D48402#1141071, @smeenai wrote:

Reid fixed the underscore issue in rL335304, so you may wanna give that a shot.

There's still a bug there: https://bugs.llvm.org/show_bug.cgi?id=39218 We don't match GCC's section name still. =/

llvm/trunk/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

public:
 
   void getNameWithPrefix(SmallVectorImpl<char> &OutName, const GlobalValue *GV,
                          const TargetMachine &TM) const override;
 };
 
 class TargetLoweringObjectFileCOFF : public TargetLoweringObjectFile {
   mutable unsigned NextUniqueID = 0;
 
+  /// Append "$symbol" to the section name when targetting mingw. The ld.bfd
+  /// COFF linker will not properly handle comdats otherwise.
+  void appendComdatSymbolForMinGW(SmallVectorImpl<char> &SecName,
+                                  StringRef Symbol, const DataLayout &DL) const;
+
 public:
   ~TargetLoweringObjectFileCOFF() override = default;
 
   void Initialize(MCContext &Ctx, const TargetMachine &TM) override;
   MCSection *getExplicitSectionGlobal(const GlobalObject *GO, SectionKind Kind,
                                       const TargetMachine &TM) const override;
 
   MCSection *SelectSectionForGlobal(const GlobalObject *GO, SectionKind Kind,

llvm/trunk/include/llvm/MC/MCAsmInfo.h

protected:
   /// True if this is a MachO target that supports the macho-specific .zerofill
   /// directive for emitting BSS Symbols.  Default is false.
   bool HasMachoZeroFillDirective = false;
 
   /// True if this is a MachO target that supports the macho-specific .tbss
   /// directive for emitting thread local BSS Symbols.  Default is false.
   bool HasMachoTBSSDirective = false;
 
+  /// True if this is a non-GNU COFF target. The COFF port of the GNU linker
+  /// doesn't handle associative comdats in the way that we would like to use
+  /// them.
+  bool HasCOFFAssociativeComdats = false;
+
   /// This is the maximum possible length of an instruction, which is needed to
   /// compute the size of an inline asm.  Defaults to 4.
   unsigned MaxInstLength = 4;
 
   /// Every possible instruction length is a multiple of this value.  Factored
   /// out in .debug_frame and .debug_line.  Defaults to 1.
   unsigned MinInstAlignment = 1;
 
public:
   bool needsDwarfSectionOffsetDirective() const {
     return NeedsDwarfSectionOffsetDirective;
   }
 
   // Accessors.
 
   bool hasMachoZeroFillDirective() const { return HasMachoZeroFillDirective; }
   bool hasMachoTBSSDirective() const { return HasMachoTBSSDirective; }
+  bool hasCOFFAssociativeComdats() const { return HasCOFFAssociativeComdats; }
   unsigned getMaxInstLength() const { return MaxInstLength; }
   unsigned getMinInstAlignment() const { return MinInstAlignment; }
   bool getDollarIsPC() const { return DollarIsPC; }
   const char *getSeparatorString() const { return SeparatorString; }
 
   /// This indicates the column (zero-based) at which asm comments should be
   /// printed.
   unsigned getCommentColumn() const { return 40; }

llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (!ComdatGV->hasPrivateLinkage()) {
       Selection = 0;
     }
   }
 
   return getContext().getCOFFSection(Name, Characteristics, Kind, COMDATSymName,
                                      Selection);
 }
 
-static const char *getCOFFSectionNameForUniqueGlobal(SectionKind Kind) {
+static StringRef getCOFFSectionNameForUniqueGlobal(SectionKind Kind) {
   if (Kind.isText())
     return ".text";
   if (Kind.isBSS())
     return ".bss";
   if (Kind.isThreadLocal())
     return ".tls$";
   if (Kind.isReadOnly() || Kind.isReadOnlyWithRel())
     return ".rdata";
   return ".data";
 }
 
+void TargetLoweringObjectFileCOFF::appendComdatSymbolForMinGW(
+    SmallVectorImpl<char> &SecName, StringRef Symbol,
+    const DataLayout &DL) const {
+  if (getTargetTriple().isWindowsGNUEnvironment()) {
+    SecName.push_back('$');
+    getMangler().getNameWithPrefix(SecName, Symbol, DL);
+  }
+}
+
 MCSection *TargetLoweringObjectFileCOFF::SelectSectionForGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   // If we have -ffunction-sections then we should emit the global value to a
   // uniqued section specifically for it.
   bool EmitUniquedSection;
   if (Kind.isText())
     EmitUniquedSection = TM.getFunctionSections();
   else
     EmitUniquedSection = TM.getDataSections();
 
   if ((EmitUniquedSection && !Kind.isCommon()) || GO->hasComdat()) {
-    const char *Name = getCOFFSectionNameForUniqueGlobal(Kind);
+    SmallString<256> Name = getCOFFSectionNameForUniqueGlobal(Kind);
+
     unsigned Characteristics = getCOFFSectionFlags(Kind, TM);
 
     Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
     int Selection = getSelectionForCOFF(GO);
     if (!Selection)
       Selection = COFF::IMAGE_COMDAT_SELECT_NODUPLICATES;
     const GlobalValue *ComdatGV;
     if (GO->hasComdat())
       ComdatGV = getComdatGVForCOFF(GO);
     else
       ComdatGV = GO;
 
     unsigned UniqueID = MCContext::GenericSectionID;
     if (EmitUniquedSection)
       UniqueID = NextUniqueID++;
 
     if (!ComdatGV->hasPrivateLinkage()) {
       MCSymbol *Sym = TM.getSymbol(ComdatGV);
       StringRef COMDATSymName = Sym->getName();
+      appendComdatSymbolForMinGW(Name, COMDATSymName,
+                                 GO->getParent()->getDataLayout());
       return getContext().getCOFFSection(Name, Characteristics, Kind,
                                          COMDATSymName, Selection, UniqueID);
     } else {
       SmallString<256> TmpData;
       getMangler().getNameWithPrefix(TmpData, GO, /*CannotUsePrivateLabel=*/true);
       return getContext().getCOFFSection(Name, Characteristics, Kind, TmpData,
                                          Selection, UniqueID);
     }
MCSection *TargetLoweringObjectFileCOFF::getSectionForJumpTable(
   // FIXME: we should produce a symbol for F instead.
   if (F.hasPrivateLinkage())
     return ReadOnlySection;
 
   MCSymbol *Sym = TM.getSymbol(&F);
   StringRef COMDATSymName = Sym->getName();
 
   SectionKind Kind = SectionKind::getReadOnly();
-  const char *Name = getCOFFSectionNameForUniqueGlobal(Kind);
+  StringRef SecName = getCOFFSectionNameForUniqueGlobal(Kind);
   unsigned Characteristics = getCOFFSectionFlags(Kind, TM);
   Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
   unsigned UniqueID = NextUniqueID++;
 
-  return getContext().getCOFFSection(Name, Characteristics, Kind, COMDATSymName,
-                                     COFF::IMAGE_COMDAT_SELECT_ASSOCIATIVE, UniqueID);
+  return getContext().getCOFFSection(
+      SecName, Characteristics, Kind, COMDATSymName,
 }
 
 void TargetLoweringObjectFileCOFF::emitModuleMetadata(MCStreamer &Streamer,
                                                       Module &M) const {
   if (NamedMDNode *LinkerOptions = M.getNamedMetadata("llvm.linker.options")) {
     // Emit the linker options to the linker .drectve section.  According to the
     // spec, this section is a space-separated string containing flags for
     // linker.

llvm/trunk/lib/MC/MCAsmInfoCOFF.cpp

MCAsmInfoCOFF::MCAsmInfoCOFF() {
   // Set up DWARF directives
   SupportsDebugInformation = true;
   NeedsDwarfSectionOffsetDirective = true;
 
   UseIntegratedAssembler = true;
 
   // At least MSVC inline-asm does AShr.
   UseLogicalShr = false;
+
+  // If this is a COFF target, assume that it supports associative comdats. It's
+  // part of the spec.
+  HasCOFFAssociativeComdats = true;
 }
 
 void MCAsmInfoMicrosoft::anchor() {}
 
 MCAsmInfoMicrosoft::MCAsmInfoMicrosoft() = default;
 
 void MCAsmInfoGNUCOFF::anchor() {}
 
-MCAsmInfoGNUCOFF::MCAsmInfoGNUCOFF() = default;
+MCAsmInfoGNUCOFF::MCAsmInfoGNUCOFF() {
+  // If this is a GNU environment (mingw or cygwin), don't use associative
+  // comdats for jump tables, unwind information, and other data associated with
+  // a function.
+  HasCOFFAssociativeComdats = false;
+}

llvm/trunk/lib/MC/MCStreamer.cpp

 static MCSection *getWinCFISection(MCContext &Context, unsigned *NextWinCFIID,
                                    MCSection *MainCFISec,
                                    const MCSection *TextSec) {
   // If this is the main .text section, use the main unwind info section.
   if (TextSec == Context.getObjectFileInfo()->getTextSection())
     return MainCFISec;
 
   const auto *TextSecCOFF = cast<MCSectionCOFF>(TextSec);
+  auto *MainCFISecCOFF = cast<MCSectionCOFF>(MainCFISec);
   unsigned UniqueID = TextSecCOFF->getOrAssignWinCFISectionID(NextWinCFIID);
 
   // If this section is COMDAT, this unwind section should be COMDAT associative
   // with its group.
   const MCSymbol *KeySym = nullptr;
-  if (TextSecCOFF->getCharacteristics() & COFF::IMAGE_SCN_LNK_COMDAT)
+  if (TextSecCOFF->getCharacteristics() & COFF::IMAGE_SCN_LNK_COMDAT) {
     KeySym = TextSecCOFF->getCOMDATSymbol();
 
-  return Context.getAssociativeCOFFSection(cast<MCSectionCOFF>(MainCFISec),
-                                           KeySym, UniqueID);
+    // In a GNU environment, we can't use associative comdats. Instead, do what
+    // GCC does, which is to make plain comdat selectany section named like
+    // ".[px]data$_Z3foov".
+    if (!Context.getAsmInfo()->hasCOFFAssociativeComdats()) {
+      std::string SectionName =
+          (MainCFISecCOFF->getSectionName() + "$" +
+           TextSecCOFF->getSectionName().split('$').second)
+              .str();
+      return Context.getCOFFSection(
+          SectionName,
+          MainCFISecCOFF->getCharacteristics() | COFF::IMAGE_SCN_LNK_COMDAT,
+          MainCFISecCOFF->getKind(), "", COFF::IMAGE_COMDAT_SELECT_ANY);
+    }
+  }
+
+  return Context.getAssociativeCOFFSection(MainCFISecCOFF, KeySym, UniqueID);
 }
 
 MCSection *MCStreamer::getAssociatedPDataSection(const MCSection *TextSec) {
   return getWinCFISection(getContext(), &NextWinCFIID,
                           getContext().getObjectFileInfo()->getPDataSection(),
                           TextSec);
 }
 

llvm/trunk/test/CodeGen/X86/mingw-comdats.ll

+; RUN: llc -mtriple=x86_64-windows-itanium < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-w64-windows-gnu < %s | FileCheck %s --check-prefix=GNU
+; RUN: llc -mtriple=x86_64-w64-windows-gnu < %s -filetype=obj | llvm-objdump - -headers | FileCheck %s --check-prefix=GNUOBJ
+
+; GCC and MSVC handle comdats completely differently. Make sure we do the right
+; thing for each.
+
+; Generated with this C++ source:
+; int bar(int);
+; __declspec(selectany) int gv = 42;
+; inline int foo(int x) { return bar(x) + gv; }
+; int main() { return foo(1); }
+
+$_Z3fooi = comdat any
+
+$gv = comdat any
+
+@gv = weak_odr dso_local global i32 42, comdat, align 4
+
+; Function Attrs: norecurse uwtable
+define dso_local i32 @main() #0 {
+entry:
+  %call = tail call i32 @_Z3fooi(i32 1)
+  ret i32 %call
+}
+
+; CHECK: main:
+; GNU: main:
+
+; Function Attrs: inlinehint uwtable
+define linkonce_odr dso_local i32 @_Z3fooi(i32 %x) #1 comdat {
+entry:
+  %call = tail call i32 @_Z3bari(i32 %x)
+  %0 = load i32, i32* @gv, align 4
+  %add = add nsw i32 %0, %call
+  ret i32 %add
+}
+
+; CHECK: .section        .text,"xr",discard,_Z3fooi
+; CHECK: _Z3fooi:
+; CHECK: .section        .data,"dw",discard,gv
+; CHECK: gv:
+; CHECK: .long 42
+
+; GNU: .section        .text$_Z3fooi,"xr",discard,_Z3fooi
+; GNU: _Z3fooi:
+; GNU: .section        .data$gv,"dw",discard,gv
+; GNU: gv:
+; GNU: .long 42
+
+; Make sure the assembler puts the .xdata and .pdata in sections with the right
+; names.
+; GNUOBJ: .text$_Z3fooi
+; GNUOBJ: .xdata$_Z3fooi
+; GNUOBJ: .data$gv
+; GNUOBJ: .pdata$_Z3fooi
+
+declare dso_local i32 @_Z3bari(i32)
+
+attributes #0 = { norecurse uwtable }
+attributes #1 = { inlinehint uwtable }