[PATCH] Alternative way of representing comdats

[David Majnemer]

================
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.
+
----------------
Rafael Ávila de Espíndola wrote:
> 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".
> 
Added a verifier check.

================
Comment at: docs/LangRef.rst:781
@@ -719,3 +780,3 @@
 .. _namedmetadatastructure:
 
 Named Metadata
----------------
Rafael Ávila de Espíndola wrote:
> 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.
> 
> 
> 
Done.

================
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.");
----------------
Rafael Ávila de Espíndola wrote:
> 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.
Done.

================
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.");
----------------
Rafael Ávila de Espíndola wrote:
> similar to the ELF helper.
Done.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:753
@@ -728,2 +752,3 @@
 }
 
+static int getSelectionForCOFF(const GlobalValue *GV) {
----------------
Rafael Ávila de Espíndola wrote:
> 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.
> 
This is possible to represent, we get this "correct":


```
SECTION HEADER #2
   .data name
       0 physical address
       0 virtual address
       4 size of raw data
      B4 file pointer to raw data (000000B4 to 000000B7)
       0 file pointer to relocation table
       0 file pointer to line numbers
       0 number of relocations
       0 number of line numbers
C0300040 flags
         Initialized Data
         4 byte align
         Read Write

SECTION HEADER #4
   .data name
       0 physical address
       0 virtual address
       4 size of raw data
      B8 file pointer to raw data (000000B8 to 000000BB)
       0 file pointer to relocation table
       0 file pointer to line numbers
       0 number of relocations
       0 number of line numbers
C0301040 flags
         Initialized Data
         COMDAT; sym= _bar
         4 byte align
         Read Write

006 00000000 SECT4  notype       Static       | .data
    Section length    4, selection    5 (pick associative Section 0x2)
009 00000000 SECT4  notype       External     | _bar
00A 00000000 SECT2  notype       External     | _foo
```

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:796
@@ +795,3 @@
+        if (!ComdatGV)
+          report_fatal_error("Associative COMDAT symbol '" + ComdatGVName +
+                             "' does not exist.");
----------------
Rafael Ávila de Espíndola wrote:
> This (along with the other non-any issues) can probably be handled earlier in the Verifier since we know that ELF only supports any.
We cannot do this because we don't know if we are targeting COFF vs ELF in the Verifier.

================
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()))
----------------
Rafael Ávila de Espíndola wrote:
> 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.
> 
Done.

http://reviews.llvm.org/D4178