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

[mstorsjo]

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?

[jeremyd2019]

This patch solves a crash https://github.com/msys2/MINGW-packages/issues/8963

[mstorsjo]

lld/COFF/InputFiles.cpp

569

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]

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

503–514

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?

534

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.

569

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]

lld/COFF/InputFiles.cpp

503–514

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

534

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

569

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]

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

[mstorsjo]

@rnk WDYT about this attempt?

[rnk]

lgtm

lld/COFF/InputFiles.cpp

void ObjFile::handleComdatSelection(
     const llvm::object::coff_aux_section_definition *def) {
   if (prevailing)
     return;
   // There's already an existing comdat for this symbol: `Leader`.
   // Use the comdats's selection field to determine if the new
   // symbol in `Sym` should be discarded, produce a duplicate symbol
   // error, etc.
 
   SectionChunk *leaderChunk = nullptr;
   COMDATType leaderSelection = IMAGE_COMDAT_SELECT_ANY;
 
   assert(leader->data && "Comdat leader without SectionChunk?");
   leaderChunk = leader->getChunk();
+  if (leaderChunk->selection == IMAGE_COMDAT_SELECT_LTO) {
+    // If the leader is only a LTO symbol, we don't know e.g. its final size
+    // yet, so we can't do the full strict comdat selection checking yet.
+    selection = leaderSelection;
+  } else {
     leaderSelection = leaderChunk->selection;
+  }
 
   if ((selection == IMAGE_COMDAT_SELECT_ANY &&
        leaderSelection == IMAGE_COMDAT_SELECT_LARGEST) ||
       (selection == IMAGE_COMDAT_SELECT_LARGEST &&
        leaderSelection == IMAGE_COMDAT_SELECT_ANY)) {
     // cl.exe picks "any" for vftables when building with /GR- and
     // "largest" when building with /GR. To be able to link object files
     // compiled with each flag, "any" and "largest" are merged as "largest".
     leaderSelection = selection = IMAGE_COMDAT_SELECT_LARGEST;
   }
 
   // GCCs __declspec(selectany) doesn't actually pick "any" but "same size as".
   // Clang on the other hand picks "any". To be able to link two object files
   // with a __declspec(selectany) declaration, one compiled with gcc and the
   // other with clang, we merge them as proper "same size as"
   if (config->mingw && ((selection == IMAGE_COMDAT_SELECT_ANY &&
                          leaderSelection == IMAGE_COMDAT_SELECT_SAME_SIZE) ||
                         (selection == IMAGE_COMDAT_SELECT_SAME_SIZE &&
                          leaderSelection == IMAGE_COMDAT_SELECT_ANY))) {
     leaderSelection = selection = IMAGE_COMDAT_SELECT_SAME_SIZE;
   }
 
   // Other than that, comdat selections must match.  This is a bit more
   // strict than link.exe which allows merging "any" and "largest" if "any"
   // is the first symbol the linker sees, and it allows merging "largest"
   // with everything (!) if "largest" is the first symbol the linker sees.
   // Making this symmetric independent of which selection is seen first
   // seems better though.
   // (This behavior matches ModuleLinker::getComdatResult().)
   if (selection != leaderSelection) {
     log(("conflicting comdat type for " + toString(*leader) + ": " +
          Twine((int)leaderSelection) + " in " + toString(leader->getFile()) +
          " and " + Twine((int)selection) + " in " + toString(this))
             .str());
     symtab->reportDuplicate(leader, this);
     return;
   }
 
   switch (selection) {
   case IMAGE_COMDAT_SELECT_NODUPLICATES:
     symtab->reportDuplicate(leader, this);
     break;
 
   case IMAGE_COMDAT_SELECT_ANY:
+  case IMAGE_COMDAT_SELECT_LTO:
     // Nothing to do.
     break;
 
   case IMAGE_COMDAT_SELECT_SAME_SIZE:
     if (leaderChunk->getSize() != getSection(sym)->SizeOfRawData) {
       if (!config->mingw) {
         symtab->reportDuplicate(leader, this);
       } else {
-        const coff_aux_section_definition *leaderDef = findSectionDef(
-            leaderChunk->file->getCOFFObj(), leaderChunk->getSectionNumber());
+        const coff_aux_section_definition *leaderDef = nullptr;
+        if (leaderChunk->file)
+          leaderDef = findSectionDef(leaderChunk->file->getCOFFObj(),
+                                     leaderChunk->getSectionNumber());
         if (!leaderDef || leaderDef->Length != def->Length)
           symtab->reportDuplicate(leader, this);
       }
     }
     break;
 
   case IMAGE_COMDAT_SELECT_EXACT_MATCH: {
     SectionChunk newChunk(this, getSection(sym));
public:
 
   coff_section section;
 };
 
 // Convenience class for initializing a SectionChunk with specific flags.
 class FakeSectionChunk {
 public:
   FakeSectionChunk(const coff_section *section) : chunk(nullptr, section) {
-    // FIXME: comdats from LTO files don't know their selection; treat them
-    // as "any".
-    chunk.selection = IMAGE_COMDAT_SELECT_ANY;
+    // Comdats from LTO files can't be fully treated as regular comdats
+    // yet; we don't e.g. know what size they are going to have, so we can't
+    // do proper checking against a regular comdat of type
+    // IMAGE_COMDAT_SELECT_SAME_SIZE. Therefore, while the symbol isn't yet
+    // LTO compiled, bail out from the nitpicky part of comdat resolution.
+    chunk.selection = IMAGE_COMDAT_SELECT_LTO;
   }
 
   SectionChunk chunk;
 };
 
 FakeSection ltoTextSection(IMAGE_SCN_MEM_EXECUTE);
 FakeSection ltoDataSection(IMAGE_SCN_CNT_INITIALIZED_DATA);
 FakeSectionChunk ltoTextSectionChunk(&ltoTextSection.section);

lld/test/COFF/lto-comdat-samesize.ll

+; REQUIRES: x86
+
+; RUN: split-file %s %t.dir
+; RUN: llvm-as %t.dir/other.ll -o %t.other.bc
+; RUN: llc -filetype=obj -o %t.main.obj %t.dir/main.ll
+
+; RUN: lld-link -out:%t.exe -subsystem:console %t.other.bc %t.main.obj
+
+#--- main.ll
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-windows-msvc19.14.0"
+
+$comdatData = comdat samesize
+
+@comdatData = weak_odr dso_local global i32 42, comdat
+
+define dso_local void @mainCRTStartup() {
+entry:
+  tail call void @other()
+  ret void
+}
+
+declare dso_local void @other()
+
+#--- other.ll
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-windows-msvc19.14.0"
+
+$comdatData = comdat samesize
+
+@comdatData = weak_odr dso_local global i32 42, comdat
+
+define dso_local void @other() {
+entry:
+  ret void
+}

llvm/include/llvm/BinaryFormat/COFF.h

 
 enum COMDATType : uint8_t {
   IMAGE_COMDAT_SELECT_NODUPLICATES = 1,
   IMAGE_COMDAT_SELECT_ANY,
   IMAGE_COMDAT_SELECT_SAME_SIZE,
   IMAGE_COMDAT_SELECT_EXACT_MATCH,
   IMAGE_COMDAT_SELECT_ASSOCIATIVE,
   IMAGE_COMDAT_SELECT_LARGEST,
-  IMAGE_COMDAT_SELECT_NEWEST
+  IMAGE_COMDAT_SELECT_NEWEST,
+  // Not a real value in files, but usable as dummy value within LLD.
+  IMAGE_COMDAT_SELECT_LTO = 0xff
 };
 
 // Auxiliary Symbol Formats
 struct AuxiliaryFunctionDefinition {
   uint32_t TagIndex;
   uint32_t TotalSize;
   uint32_t PointerToLinenumber;
   uint32_t PointerToNextFunction;