[PATCH] D56929: lld-link: Use just one code path to process associative comdats, reject some invalid associated comdats

Tue Jan 22 13:28:13 PST 2019

pcc added a comment.

> Currently, if an associative comdat appears after the comdat it's associated with it's processed immediately, else it's deferred until the end of the object file. I found this confusing to think about while working on PR40094, so this patch makes it so that associated comdats are always processed at the end of the object file. Tests seem happy, are there problems with this?

Nope, it was a (premature, as it turns out) optimization. According to my benchmarking, this patch is performance neutral linking chrome_child.dll.

================
Comment at: lld/COFF/InputFiles.cpp:239
+    COFFObj->getSectionName(ParentSec, ParentName);
+    error("associative comdat " + Name +
+          " refers to associative comdat " + ParentName);
----------------
I think this is not the only reason why we might end up here. We could also get here if an `IMAGE_SCN_LNK_COMDAT` section does not have any symbols at all (or just has a section definition) and another section is associative with it. I reckon it's probably not worth trying to disambiguate these cases in code so maybe the diagnostic should just say `invalid COFF file`? (I am slightly inclined to say that we shouldn't try to diagnose at all here and should just let the linker crash.)

================
Comment at: lld/COFF/InputFiles.cpp:339
       // 2) symbol belongs to a comdat section associated with a section whose
       //    section definition symbol appears later in the symbol table.
       // In both of these cases, we can expect the section to be resolved by
----------------
Comment needs to be updated.

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

https://reviews.llvm.org/D56929