This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Drop GRP_COMDAT if the group signature is localized
ClosedPublic

Authored by MaskRay on Jul 25 2021, 10:09 PM.

Details

Summary

See GRP_COMDAT group with STB_LOCAL signature
objcopy PR: https://sourceware.org/bugzilla/show_bug.cgi?id=27931

GRP_COMDAT deduplication is purely based on the signature symbol name in
ld.lld/GNU ld/gold. The local/global status is not part of the equation.

If the signature symbol is localized by --localize-hidden or
--keep-global-symbol, the intention is likely to make the group fully
localized. Drop GRP_COMDAT to suppress deduplication.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 25 2021, 10:09 PM
MaskRay requested review of this revision.Jul 25 2021, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2021, 10:09 PM
MaskRay edited the summary of this revision. (Show Details)Jul 25 2021, 10:17 PM
jhenderson added inline comments.Jul 26 2021, 12:09 AM
llvm/test/tools/llvm-objcopy/ELF/group.test
82

"is" confused me slightly, suggesting it was the original state of the symbol.

90
91

This line is testing the yaml2obj output, which I don't think is what you want. Did you mean to dump %t1 or do something else?

llvm/tools/llvm-objcopy/ELF/Object.cpp
1074–1075

Possibly worth adding a reference to the gABI list discussion for further justification?

1076–1077

If I'm reading this correctly, this will happen even if no operations have been performed to make the symbol STB_LOCAL (i.e. it was STB_LOCAL in the input). Perhaps we should have an explicit test case for that too.

MaskRay updated this revision to Diff 361584.Jul 26 2021, 12:28 AM
MaskRay marked 5 inline comments as done.

address comments

llvm/test/tools/llvm-objcopy/ELF/group.test
91

Ah, right. I intended to test %t1

jhenderson accepted this revision.Jul 26 2021, 12:32 AM

LGTM, with two nits.

llvm/test/tools/llvm-objcopy/ELF/group.test
120

Seems like you can drop this? (possibly also the Link field, but I don't remember what the default is for group sections).

127

Perhaps simplify to just [SHF_GROUP]?

This revision is now accepted and ready to land.Jul 26 2021, 12:32 AM
MaskRay updated this revision to Diff 361586.Jul 26 2021, 12:34 AM

simplify test

This revision was landed with ongoing or failed builds.Jul 26 2021, 9:05 AM
This revision was automatically updated to reflect the committed changes.

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

A latent problem triggered by the change. Fixed by c5d8bd5a35cbd325c6ccd42afa91bad06d261f07

Looks like this caused a buildbot failure on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/5110

A latent problem triggered by the change. Fixed by c5d8bd5a35cbd325c6ccd42afa91bad06d261f07

Thanks! The build bot is back to green now.