[PATCH] D157136: [LLD][COFF] Handle 'label' symbols when they point to a COMDAT section

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Here are some comments, I have to run, so it's not complete, but hopefully still useful.



================
Comment at: lld/COFF/InputFiles.cpp:388
   DenseMap<StringRef, uint32_t> prevailingSectionMap;
-  std::vector<const coff_aux_section_definition *> comdatDefs(
-      coffObj->getNumberOfSections() + 1);
+  std::vector<std::pair<DefinedRegular *, const coff_aux_section_definition *>>
+      comdatDefs(coffObj->getNumberOfSections() + 1);
----------------
This array is sparse, so we are adding a pointer for every input section, so this could be a performance sensitive change.


================
Comment at: lld/COFF/InputFiles.cpp:645
+      // the label to point to the leader' section.
+      if (sym.isLabel()) {
+        return make<DefinedRegular>(this, /*Name*/ "", /*IsCOMDAT*/ false,
----------------
For my understanding, the problem is that the old code just returned the leader in this case, right? And you have solved the problem by synthesizing a new symbol to represent the label.


================
Comment at: lld/test/COFF/Inputs/comdat-malformed-assoc-a.yaml:55
+      Selection:       IMAGE_COMDAT_SELECT_ANY
+  - Name:            '.text$x'
+    Value:           0
----------------
I see, `.text$x` is not comdat but `.text$mn` is. I think this test would be more readable in assembly. If you start with assembly output from clang, can you adjust the section directives to create the analogous situation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157136