[LLD][COFF] Enable linking of __declspec(selectany) symbols from Clang and GCC

[zero9178]

Summary

When annotating a symbol with declspec(selectany), Clang assigns it comdat 2 while GCC assigns it comdat 3. This patch enables two object files that contain a declspec(selectany) symbol, one created by gcc and the other by clang, to be linked together instead of issuing a duplicate symbol error.

Would need someone to commit this for me if accepted.

[mstorsjo]

This needs a testcase. Have a look in lld/test/COFF, it's probably most suitable with a .s test for this (the main test file itself simulating the object file created by e.g. clang, and then either a separate file in test/COFF/Inputs, or generating that second file with echo statements within the file (there's a few examples of this among the existing tests), for the other file - assemble both and try to link them together.

[mstorsjo]

There might be existing tests for this kind of conflict - in that case it's probably easiest to extend it to check that there's no conflict if the mingw flag is set.

[rnk]

In D73139#1832396, @mstorsjo wrote:

This needs a testcase. Have a look in lld/test/COFF, it's probably most suitable with a .s test for this (the main test file itself simulating the object file created by e.g. clang, and then either a separate file in test/COFF/Inputs, or generating that second file with echo statements within the file (there's a few examples of this among the existing tests), for the other file - assemble both and try to link them together.

That sounds right to me. You know, I wonder if this would work for test structure:

# RUN: llvm-mc %s -defsym obj=1 -filetype=obj -o %t1.obj
# RUN: llvm-mc %s -defsym obj=2 -filetype=obj -o %t2.obj
# RUN: lld-link ... %t1.obj %t2.obj

.if obj=1
# obj1 contents
.endif

.if obj2
# obj2 contents
.endif

I haven't tried it, but .if seems nicer than echo escapes.

lld/COFF/InputFiles.cpp

511

Does ld.bfd implement the same size check? If so, it might be more conservative to use IMAGE_COMDAT_SELECT_SAME_SIZE for the leader.

I see that GCC only allows this attribute to apply to data, not code, so "same size" makes sense, even if it ignores that "selectany" really means IMAGE_COMDAT_SELECT_ANY. :)

[mstorsjo]

In D73139#1832587, @rnk wrote:

That sounds right to me. You know, I wonder if this would work for test structure:

# RUN: llvm-mc %s -defsym obj=1 -filetype=obj -o %t1.obj
# RUN: llvm-mc %s -defsym obj=2 -filetype=obj -o %t2.obj
# RUN: lld-link ... %t1.obj %t2.obj

.if obj=1
# obj1 contents
.endif

.if obj2
# obj2 contents
.endif

I haven't tried it, but .if seems nicer than echo escapes.

Nice! That seems to work indeed, but the condition needs to be .if obj==1. I'll try to keep it in mind the next time I need a multi-file testcase myself as well.

[zero9178]

Added tests to confirm behavior. Changed the resulting comdat to be "same size as" instead of "any". This matches ld.bfd's behavior

[mstorsjo]

Looks good to me, but I'll leave it open for others to comment on still.

lld/test/COFF/comdat-gcc-compatibility.s

3

FYI /nodefaultlib isn't strictly needed here, as it only affects cases where the object files have embedded /defaultlib: directives.

[zero9178]

Removed /:nodefaultlib. Applied formatting

[mstorsjo]

lld/test/COFF/comdat-gcc-compatibility.s

2

Sorry I forgot to mention before, this needs a REQUIRES: x86 line, otherwise it'll fail on builds with the x86 backend disabled.

[zero9178]

Added X86 requirement

[rnk]

lgtm

[zero9178]

Thanks! Would need someone to commit this for me

[mstorsjo]

@hans - I'd like to nominate this for the 10.0 release branch. We're early in the branch, the change should be pretty safe from regressions (it loosens a check that we knew was stricter than other linkers, and only does it for the mingw case).

[hans]

In D73139#1835611, @mstorsjo wrote:

@hans - I'd like to nominate this for the 10.0 release branch. We're early in the branch, the change should be pretty safe from regressions (it loosens a check that we knew was stricter than other linkers, and only does it for the mingw case).

Sounds good to me. Cherry-picked in 5d37ce7e19c9961b45bef2f2f66805f4a8f02daf. Please let me know if there are any follow-ups.

lld/COFF/InputFiles.cpp

     leaderSelection = selection = IMAGE_COMDAT_SELECT_LARGEST;
   }
 
+  // GCCs __declspec(selectany) doesn't actually pick "any" but "same size as".
+  // Clang on the other hand picks "any". To be able to link two object files
+  // with a __declspec(selectany) declaration, one compiled with gcc and the other
+  // with clang, we merge them as proper "same size as"
+  if (config->mingw && ((selection == IMAGE_COMDAT_SELECT_ANY &&
+                         leaderSelection == IMAGE_COMDAT_SELECT_SAME_SIZE) ||
+                        (selection == IMAGE_COMDAT_SELECT_SAME_SIZE &&
+                         leaderSelection == IMAGE_COMDAT_SELECT_ANY))) {
+    leaderSelection = selection = IMAGE_COMDAT_SELECT_SAME_SIZE;
+  }
+
   // Other than that, comdat selections must match.  This is a bit more
   // strict than link.exe which allows merging "any" and "largest" if "any"
   // is the first symbol the linker sees, and it allows merging "largest"

lld/test/COFF/comdat-gcc-compatibility.s

+# RUN: llvm-mc %s -triple x86_64-pc-win32 -defsym obj=0 -filetype=obj -o %t1.obj
+# RUN: llvm-mc %s -triple x86_64-pc-win32 -defsym obj=1 -filetype=obj -o %t2.obj
+# RUN: lld-link /lldmingw /noentry /nodefaultlib /dll %t1.obj %t2.obj /out:%t3.dll
+# RUN: not lld-link /noentry /nodefaultlib /dll %t1.obj %t2.obj /out:%t3.dll
+.if obj==0
+        .section .text$nm, "", discard, symbol
+.else
+        .section .text$nm, "", same_size, symbol
+.endif
+        .globl symbol
+symbol:
+        .long 1