[PATCH] Alternative way of representing comdats

[Rafael Ávila de Espíndola]

almost there I think :-)

================
Comment at: docs/LangRef.rst:778
@@ +777,3 @@
+``IMAGE_COMDAT_SELECT_ASSOCIATIVE`` which is associated with the first COMDAT
+section and contains the contents of the ``@baz`` symbol.
+
----------------
OK, I get it in what way the comdat name is significant. The linker will pick the section whose comdat symbol is the largest, not the largest section. Can you think of a way to document this a bit more explicitly? If not, don't worry. I assume this should be clear to anyone familiar with COFF.

It does suggest that for any selection other than any we should require a real symbol to be present. For example, we should error on

  $bar = comdat largest
  @foo = global i32 42, comdat $bar

saying "no matching symbol for $bar, cannot decide which one is largest".


================
Comment at: docs/LangRef.rst:781
@@ -719,3 +780,3 @@
 .. _namedmetadatastructure:
 
 Named Metadata
----------------
What I meant about IR sections being split into multiple object sections is that in, for example,

  $foo = comdat any
  $bar = comdat any
  @g1 = global i32 42, section "sec", comdat $foo
  @g2 = global i32 42, section "sec", comdat $bar

there is only one section "sec", but in the object file we end up with two sections named sec:

  [ 6] sec               PROGBITS        0000000000000000 000050 000004 00 WAG  0   0  4
  [ 7] sec               PROGBITS        0000000000000000 000054 000004 00 WAG  0   0  4



One last thing that we might want to document is that CodeGen can create comdats even if no comdat is explicit in the IR. For example, given

  @bar = weak global i32 42

we create a comdat and put bar in it. This is something we might want to change now that we have proper comdat support, but for now it is better to just document it.




================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:262
@@ +261,3 @@
+        if (C->getSelectionKind() != Comdat::Any)
+          report_fatal_error("ELF COMDATs only support SelectionKind::Any, '" +
+                             C->getName() + "' cannot be lowered.");
----------------
I think you can avoid the duplicated error reporting with a static helper:

const Comdat *getELFComdat(const GlobalValue &GV)

it returns null if there is no comdat and reports a fatal error it there is one with a seleciton other than any.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:584
@@ +583,3 @@
+  if (const Comdat *C = GV->getComdat())
+    report_fatal_error("MachO doesn't support COMDATs, '" + C->getName() +
+                       "' cannot be lowered.");
----------------
similar to the ELF helper.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:753
@@ -728,2 +752,3 @@
 }
 
+static int getSelectionForCOFF(const GlobalValue *GV) {
----------------
It looks like for COFF we miscompile

  $foo = comdat any
  @bar = global i32 42, comdat $foo
  @foo = global i32 42

Is that even possible to represent in COFF? Since "foo" is a comdat key, the symbol "foo" has to be in the section that has that comdat.I guess in an object file you could have two symbols named foo, but that would be hard to represent in assembly.or even at the IR for selections other than any.

Can we just error in codegen when trying to output that for COFF? It does work for ELF, so it would probably be an interesting testcase to have.


================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:796
@@ +795,3 @@
+        if (!ComdatGV)
+          report_fatal_error("Associative COMDAT symbol '" + ComdatGVName +
+                             "' does not exist.");
----------------
This (along with the other non-any issues) can probably be handled earlier in the Verifier since we know that ELF only supports any.

================
Comment at: lib/Linker/LinkModules.cpp:549
@@ +548,3 @@
+  GlobalValue *GV = M->getNamedValue(ComdatName);
+  if (auto GA = dyn_cast<GlobalAlias>(GV))
+    if (auto GO = dyn_cast<GlobalObject>(GA->getAliasee()->stripPointerCasts()))
----------------
Is the alias handling correct? They don't exist in the object file (they are just another symbol). So I assume that given

  $foo = comdat largest
  @foo = alias  getelementptr([2 x i32]* @bar, i32 0, i32 1)
  @bar = global [2 x i32] zeroinitializer, comdat $foo

The linker will end up seeing that foo has a size of 32 bits.

Can we just error in here for now? Something along the lines of "cannot compute size of this alias, so cannot select one comdat". We can extend it afterwards for cases where we are able to compute the size, like simple geps.

http://reviews.llvm.org/D4178