Page MenuHomePhabricator

Fix logic for which symbols to keep with comdats
ClosedPublic

Authored by rafael on Mar 23 2016, 4:43 AM.

Details

Summary

If a comdat is dropped, all symbols in it are dropped.
If a comdat is kept, the symbols survive to pass regular symbol resolution.
With this patch we do that for all global symbols.

The added test is a copy of test/tools/gold/X86/comdat.ll that we now pass.

Diff Detail

Event Timeline

rafael updated this revision to Diff 51399.Mar 23 2016, 4:43 AM
rafael retitled this revision from to Fix logic for which symbols to keep with comdats.
rafael updated this object.
rafael added reviewers: tejohnson, mehdi_amini.
rafael added a subscriber: llvm-commits.

comment for phab to send email to the list.

tejohnson edited edge metadata.Mar 23 2016, 1:06 PM

I'm not sure how these changes are fixing the issue described - we used to always add to ValuesToLink any GV in a chosen comdat, but here we only sometimes do. Can't that result in incomplete comdat groups?

I tried the new test case with HEAD and see that it doesn't have a $c1 comdat at all, but not sure why or how that is fixed by the patch. I'm obviously missing something, can you give me a better idea of how this is working?

lib/Linker/LinkModules.cpp
425–426

We now only add comdat members to ValuesToLink on a case by case basis below.

567–568

Why only internal members? If we have a comdat member in ValuesToLink (added in linkIfNeeded), then presumably we had selected that copy of the comdat and wouldn't we want all members?

test/Linker/Inputs/comdat16.ll
6

Should test check that?

rafael updated this revision to Diff 51465.Mar 23 2016, 1:20 PM
rafael edited edge metadata.

Update test.

tejohnson accepted this revision.Mar 23 2016, 2:06 PM
tejohnson edited edge metadata.
This revision is now accepted and ready to land.Mar 23 2016, 2:06 PM
rafael closed this revision.Mar 23 2016, 2:22 PM