Page MenuHomePhabricator

Fix logic for which symbols to keep with comdats

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



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?


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


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?


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