Page MenuHomePhabricator

[LLD] [COFF] Fix handling of LTO comdats with nontrivial selection types after 728cc0075e5dfdb454eb
ClosedPublic

Authored by mstorsjo on Jun 20 2021, 2:19 PM.

Details

Summary

Commit 728cc0075e5dfdb454ebe6418b63bdffae71ec14 made comdat symbols
from LTO objects be treated as any regular comdat symbol. This works
great for symbols that actually are IMAGE_COMDAT_SELECT_ANY, but
if e.g. the symbols have a less trivial selection type, like
IMAGE_COMDAT_SELECT_SAME_SIZE, we can't check that between the plain
symbol from the bitcode file (before actually doing the LTO compilation)
and other regular object files.

Therefore bring back one aspect of handling from before; that comdat
resolution with a leader from an LTO symbol is essentially skipped,
like it was before 728cc0075e5dfdb454ebe6418b63bdffae71ec14.

Or is there any better way of including the info about the kind of
symbol for the leader? Previously we could check leader->data to see
if it was a regular or LTO symbol. Overall we'd of course want to make
LTO cases behave more like regular symbols, but for this aspect, I
don't think we practically can?

Diff Detail

Event Timeline

mstorsjo created this revision.Jun 20 2021, 2:19 PM
mstorsjo requested review of this revision.Jun 20 2021, 2:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2021, 2:19 PM
mstorsjo added inline comments.Jun 21 2021, 12:53 PM
lld/COFF/InputFiles.cpp
565

Also just for reference, this particular bit of the patch fixes the crash that @jeremyd2019 mentioned. (The testcase doesn't exactly test this situation though, but I'm including the fix as fairly trivial.) This particular change alone would degrade the crash into an error about duplicate symbols (due to comdat mismatches), while the rest of the patch makes the situation not an error, like before.

rnk added a comment.Jun 21 2021, 1:42 PM

Or is there any better way of including the info about the kind of
symbol for the leader? Previously we could check leader->data to see
if it was a regular or LTO symbol. Overall we'd of course want to make
LTO cases behave more like regular symbols, but for this aspect, I
don't think we practically can?

I'm not sure. One other way you could test if a symbol is an LTO symbol is to check if it comes from a BitcodeFile.

lld/COFF/InputFiles.cpp
502–512

Can this be simplified a bit to merge declaration and assignment? Maybe something like:

SectionChunk *leaderChunk = leader->getChunk();
COMDATType leaderSelection = leaderChunk->selection;
if (leaderSelection == IMAGE_COMDAT_SELECT_LTO)
  leaderSelection = selection = IMAGE_COMDAT_SELECT_ANY;

I'm not 100% sure that is the same, but there's something there.


Actually, maybe the simplest way to recover the old behavior is to check isa<BitcodeFile>(leader->file). Then a new COMDATType isn't needed. Does that work?

531

I suppose a different fix could be to weaken same_size to any here, but that would erode some of the value of same_size checking.

565

I see, right, so GCC regular objects use same_size, Clang LTO uses any, and the combination results in same_size, and we get here.

mstorsjo added inline comments.Jun 21 2021, 2:09 PM
lld/COFF/InputFiles.cpp
502–512

I'll give it a shot at using isa<BitcodeFile>() - it would be nice to not need to add fictive enum members.

531

Additionally, that wouldn't fix the particular testcase that this patch adds, which runs entirely without -lldmingw.

565

Yep, exactly. This particular situation with the GCC same_size selection is kind of a red herring, as we'd have the same issue with consistently used same_size in an all-clang setup, if one object is LTO and one is regular.

mstorsjo updated this revision to Diff 353490.Jun 21 2021, 2:23 PM

Use @rnk's suggsestion of isa<BitcodFile>(), simplify the initialization of leaderChunk and leaderSelection.

@rnk WDYT about this attempt?

rnk accepted this revision.Jun 24 2021, 3:30 PM

lgtm

This revision is now accepted and ready to land.Jun 24 2021, 3:30 PM