[PATCH] Alternative way of representing comdats

[Rafael Ávila de Espíndola]

This also needs tests for the errors. In particular

* comdat that is only a forward ref
* redefinition of a comdat.
* linker errors

And one more thing, when dropping a comdat we should drop it from the comdat symbol table.

================
Comment at: docs/LangRef.rst:732
@@ +731,3 @@
+specify this key will only end up in the final object file if the linker chooses
+that key over some other key.
+
----------------
Other set of objects/sections with that key.

"the linker" in here is a bit ambiguous. Are we talking about the system linker or lib/Linker. Since we produce relocatable objects, it is probably better to state this in terms of what we do: put the global objects in a section with that comdat.


================
Comment at: docs/LangRef.rst:734
@@ +733,3 @@
+
+Note that the Mach-O platform doesn't support COMDATs and ELF only supports
+``any`` as a selection kind.
----------------
Move this after the part the defines what the selection kind is maybe?

================
Comment at: docs/LangRef.rst:761
@@ +760,3 @@
+
+Note that not all targets support COMDATs and some may only support ``any`` as a
+selection kind.
----------------
This paragraph is now redundant.

================
Comment at: docs/LangRef.rst:763
@@ +762,3 @@
+selection kind.
+
+Here is an example of a COMDAT group where a function will only be selected if
----------------
There are still some things should probably be mentioned:

* We split "IR level sections" into multiple sections in the final .o since in object files you can't have a section that is part in one comdat and part in another.
* Explain how we map mulitple sections with the same comdat to using associative comdts on COFF. Otherwise someone is sure to look at the list and think associative is missing :-)
* Alias are placed in the same comdat the aliasee computes to, if any.

================
Comment at: include/llvm/ADT/UniqueVector.h:24
@@ -23,2 +23,3 @@
 /// Entries can be fetched using operator[] with the entry ID.
 template<class T> class UniqueVector {
+public:
----------------
Cool, I had no idea we had this :-)

================
Comment at: include/llvm/IR/GlobalValue.h:115
@@ +114,3 @@
+  bool hasComdat() const { return getComdat() != nullptr; }
+  Comdat *getComdat() {
+    return const_cast<Comdat *>(
----------------
This looks odd. You probably want the const version to forward to the non-const:

return const_cast<GlobalValue*>(this)->getComdat();


================
Comment at: include/llvm/Support/GCOV.h:369
@@ +368,3 @@
+          BranchesExec(0), BranchesTaken(0) {}
+    GCOVCoverage(StringRef Name)
+        : Name(Name), LogicalLines(0), LinesExec(0), Branches(0),
----------------
unrelated change?

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:252
@@ -242,4 +251,3 @@
       !Kind.isCommon()) {
-    const char *Prefix;
-    Prefix = getSectionPrefixForGlobal(Kind);
+    StringRef Prefix = getSectionPrefixForGlobal(Kind);
 
----------------
Nice cleanup. You can commit this now if you want.

================
Comment at: lib/CodeGen/TargetLoweringObjectFileImpl.cpp:844
@@ -781,1 +843,3 @@
+  if ((GV->isWeakForLinker() || EmitUniquedSection || GV->hasComdat()) &&
+      !Kind.isCommon()) {
     const char *Name = getCOFFSectionNameForUniqueGlobal(Kind);
----------------
Reminds me: we should probably reject GlobalObject with common linkage and an explicit comdat in the verifier.

================
Comment at: lib/Linker/LinkModules.cpp:542
@@ -536,1 +541,3 @@
 
+static GlobalValue *getComdatLeader(Module *M, StringRef ComdatName) {
+  GlobalValue *GV = M->getNamedValue(ComdatName);
----------------
Not sure I follow. Is this assuming that the comdat name is related to the name of a global? That may be a common use but is not in any way required by the semantics.


================
Comment at: lib/Linker/LinkModules.cpp:558
@@ +557,3 @@
+  if (DstC && SrcC) {
+    Comdat::SelectionKind DSK = DstC->getSelectionKind();
+    Comdat::SelectionKind SSK = SrcC->getSelectionKind();
----------------
I assume this logic matches what the COFF linker does? Can you extract it to a helper like

bool computeResultingSeelectionKind(Comdat::SelectionKind &Result, Comdat::SelectionKind &Src, Comdat::SelectionKind &Dst) ...

and put a comment saying where the merging logic comes from.

================
Comment at: lib/Linker/LinkModules.cpp:583
@@ +582,3 @@
+    case Comdat::SelectionKind::Newest:
+      // Go with src
+      LinkFromSrc = true;
----------------
This is fairly arbitrary for Newest. We should probably error or actually check the bitcode timestamp, no?

================
Comment at: lib/Linker/LinkModules.cpp:595
@@ +594,3 @@
+
+      const auto SrcGV = dyn_cast<GlobalVariable>(SrcGVLeader);
+      const auto DstGV = dyn_cast<GlobalVariable>(DstGVLeader);
----------------
"auto *" instead of "auto" is probably better since it makes it obvious it is a pointer.

================
Comment at: lib/Linker/LinkModules.cpp:626
@@ +625,3 @@
+        return emitError("Linking COMDATs named '" + ComdatName +
+                         "': ExactMatch requries GlobalVariable");
+      }
----------------
Not just ExactMatch.

================
Comment at: lib/Linker/LinkModules.cpp:865
@@ +864,3 @@
+  bool LinkFromSrc = false;
+  Comdat *DC = nullptr;
+  if (const Comdat *SC = SGV->getComdat()) {
----------------
BTW, do we produce as error if we have

module1:

@a = ... comdat $c1, largest

module2

@b = ... comdat $c1, newest

that is, we have conflicting comdat selection kinds, but the comdats never show up in the corresponding global objects.

http://reviews.llvm.org/D4178