i686 mingw-w64 comdat functions use wrong .text section name

[rnk]

Summary

Fix for bug 39218

This patch seems to fix this issue. However, from a code standpoint, I'm not sure it is up to standards. I am submitting this to see if it is directionally correct, and seeking feedback for a better way to accomplish this.

Questions I have:

1. Are the modifications to the test correct? The code seems to function (it passes check-llvm, check-clang, and manual testing of the example with GCC). I am not very familiar with comdats or COFF, and therefore I am not 100% confident in the test case.
2. Is GO->getComdat()->getName() the correct place to get the name, or is it a coincidence that it contains the right data?

Any feedback is welcome, and I am happy to submit another patch incorporating any suggested changes.

[rnk]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1318–1325

I think this needs a bit of massaging. I don't think we want to check for isArch32Bit, we should just always use the name of the comdat if it's available. However, GO->getComdat() may return null, and this code should fall back to getting the IR name of the global in that case. I'll try to put something together for this later today.

[mstorsjo]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1318–1325

Also, when it comes to "weird name mangling" on windows, the differentiator isn't usually whether it is 32 bit, but i386 alone has got the weird cases, 32 bit arm is just as simple/straightforward as x86_64 and arm64 when it comes to name mangling.

[rnk]

Let me grab this to post the updated patch...

[rnk]

• generalize things slightly

[rnk]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

1318–1325

Does this seem correct, @mstorsjo?

[mstorsjo]

LGTM - this looks sensible to me. I haven't verified the original issue and whether this fixes it, but the change itself looks good.

[rnk]

In D54762#1305746, @mstorsjo wrote:

LGTM - this looks sensible to me. I haven't verified the original issue and whether this fixes it, but the change itself looks good.

I never could reproduce the linker error either. =/ I can easily check the section name that GCC uses, but using the wrong section names doesn't seem to cause any problems, so I didn't prioritize fixing this. Anyway, better to match behavior when it's otherwise inconsequential.

[• herman6067]

In D54762#1305765, @rnk wrote:

I never could reproduce the linker error either. =/ I can easily check the section name that GCC uses, but using the wrong section names doesn't seem to cause any problems, so I didn't prioritize fixing this. Anyway, better to match behavior when it's otherwise inconsequential.

I was able to create the linker error using gcc 8.1.0 from mingw-builds.org. I've rebuilt clang with these changes and it seems to be working well now. Thank you very much!

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

else
       ComdatGV = GO;
 
     unsigned UniqueID = MCContext::GenericSectionID;
     if (EmitUniquedSection)
       UniqueID = NextUniqueID++;
 
     if (!ComdatGV->hasPrivateLinkage()) {
       MCSymbol *Sym = TM.getSymbol(ComdatGV);
       StringRef COMDATSymName = Sym->getName();
 
-      // Append "$symbol" to the section name when targetting mingw. The ld.bfd
+      // Append "$symbol" to the section name *before* IR-level mangling is
+      // applied when targetting mingw. This is what GCC does, and the ld.bfd
       // COFF linker will not properly handle comdats otherwise.
       if (getTargetTriple().isWindowsGNUEnvironment())
-        raw_svector_ostream(Name) << '$' << COMDATSymName;
+        raw_svector_ostream(Name) << '$' << ComdatGV->getName();
 
       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);
     }

llvm/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=i686-w64-windows-gnu < %s | FileCheck %s --check-prefix=GNU32
-; RUN: llc -mtriple=x86_64-w64-windows-gnu < %s -filetype=obj | llvm-objdump - -headers | FileCheck %s --check-prefix=GNUOBJ
+; RUN: llc -function-sections -mtriple=x86_64-windows-itanium < %s | FileCheck %s
+; RUN: llc -function-sections -mtriple=x86_64-windows-msvc < %s | FileCheck %s
+; RUN: llc -function-sections -mtriple=x86_64-w64-windows-gnu < %s | FileCheck %s --check-prefix=GNU
+; RUN: llc -function-sections -mtriple=i686-w64-windows-gnu < %s | FileCheck %s --check-prefix=GNU32
+; RUN: llc -function-sections -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:
+; Modeled on this C++ source, with additional modifications for
+; -ffunction-sections:
 ; 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: .section        .text,"xr",one_only,main
 ; CHECK: main:
+; GNU: .section        .text$main,"xr",one_only,main
 ; GNU: main:
+; GNU32: .section        .text$main,"xr",one_only,_main
+; GNU32: _main:
+
+define dso_local x86_fastcallcc i32 @fastcall(i32 %x, i32 %y) {
+  %rv = add i32 %x, %y
+  ret i32 %rv
+}
+
+; CHECK: .section        .text,"xr",one_only,fastcall
+; CHECK: fastcall:
+; GNU: .section        .text$fastcall,"xr",one_only,fastcall
+; GNU: fastcall:
+; GNU32: .section        .text$fastcall,"xr",one_only,@fastcall@8
+; GNU32: @fastcall@8:
 
 ; 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
 
-; GNU32: .section        .text$__Z3fooi,"xr",discard,__Z3fooi
+; GNU32: .section        .text$_Z3fooi,"xr",discard,__Z3fooi
 ; GNU32: __Z3fooi:
-; GNU32: .section        .data$_gv,"dw",discard,_gv
+; GNU32: .section        .data$gv,"dw",discard,_gv
 ; GNU32: _gv:
 ; GNU32: .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 }