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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

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



================
Comment at: lld/COFF/InputFiles.cpp:503-514
   SectionChunk *leaderChunk = nullptr;
   COMDATType leaderSelection = IMAGE_COMDAT_SELECT_ANY;
 
   assert(leader->data && "Comdat leader without SectionChunk?");
   leaderChunk = leader->getChunk();
-  leaderSelection = leaderChunk->selection;
+  if (leaderChunk->selection == IMAGE_COMDAT_SELECT_LTO) {
+    // If the leader is only a LTO symbol, we don't know e.g. its final size
----------------
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?


================
Comment at: lld/COFF/InputFiles.cpp:534
                          leaderSelection == IMAGE_COMDAT_SELECT_ANY))) {
     leaderSelection = selection = IMAGE_COMDAT_SELECT_SAME_SIZE;
   }
----------------
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.


================
Comment at: lld/COFF/InputFiles.cpp:569
+        const coff_aux_section_definition *leaderDef = nullptr;
+        if (leaderChunk->file)
+          leaderDef = findSectionDef(leaderChunk->file->getCOFFObj(),
----------------
mstorsjo wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104605/new/

https://reviews.llvm.org/D104605