Page MenuHomePhabricator

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

Authored by zero9178 on Jan 21 2020, 1:39 PM.

Details

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.

Diff Detail

Event Timeline

zero9178 created this revision.Jan 21 2020, 1:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2020, 1:39 PM
zero9178 edited the summary of this revision. (Show Details)Jan 21 2020, 1:41 PM
zero9178 edited the summary of this revision. (Show Details)
zero9178 edited the summary of this revision. (Show Details)

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.

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 added a comment.Jan 21 2020, 3:27 PM

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. :)

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 updated this revision to Diff 239559.Jan 22 2020, 6:14 AM

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

zero9178 marked an inline comment as done.Jan 22 2020, 6:14 AM

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

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

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

zero9178 updated this revision to Diff 239611.EditedJan 22 2020, 8:39 AM

Removed /:nodefaultlib. Applied formatting

zero9178 marked an inline comment as done.Jan 22 2020, 8:39 AM
mstorsjo added inline comments.Jan 22 2020, 9:33 AM
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 updated this revision to Diff 239649.Jan 22 2020, 10:35 AM

Added X86 requirement

zero9178 marked an inline comment as done.Jan 22 2020, 10:35 AM
rnk accepted this revision.Jan 22 2020, 11:29 AM

lgtm

This revision is now accepted and ready to land.Jan 22 2020, 11:29 AM

Thanks! Would need someone to commit this for me

This revision was automatically updated to reflect the committed changes.
mstorsjo added a subscriber: hans.Jan 23 2020, 1:07 AM

@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 added a comment.Jan 23 2020, 9:26 AM

@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.