[PATCH] sectiongroup support

[kledzik at apple.com]

================
Comment at: lib/Core/SymbolTable.cpp:128-130
@@ +127,5 @@
+// Return the group for an atom.
+const Atom *SymbolTable::getGroup(const Atom &childAtom) {
+  const DefinedAtom *definedAtom = llvm::dyn_cast<DefinedAtom>(&childAtom);
+  for (const Reference *r : *definedAtom) {
+    if (r->kindNamespace() != Reference::KindNamespace::all)
----------------
This needs to handle the case where the childAtom is not a DefinedAtom.  Currently it will crash because definedAtom will be NULL.

================
Comment at: lib/Core/SymbolTable.cpp:192-196
@@ +191,7 @@
+        // If both atoms were part of a group with the same signature
+        const Atom *signatureAtomForExisting = getGroup(*existing);
+        const Atom *signatureAtomForNew = getGroup(newAtom);
+        if (signatureAtomForExisting && signatureAtomForNew) {
+          if (signatureAtomForExisting == signatureAtomForNew)
+            return;
+          if (signatureAtomForExisting->name() == signatureAtomForNew->name())
----------------
The return here seems wrong.  The code should always make it to the bottom of the function which puts one of the two into the replacement table.  But for the return to happen, you'd need to atoms with the same name in the same group.  Is that even possible?  Maybe it should just be:
    assert(signatureAtomForExisting != signatureAtomForNew);

================
Comment at: lib/Core/Resolver.cpp:436-444
@@ +435,11 @@
+      return false;
+    if ((a->definition() == Atom::definitionRegular &&
+         replacedAtom->definition() == Atom::definitionRegular)) {
+      const DefinedAtom *iterDefAtom = llvm::dyn_cast<DefinedAtom>(a);
+      const DefinedAtom *replacedDefAtom =
+          llvm::dyn_cast<DefinedAtom>(replacedAtom);
+      // If both atoms are of type COMDAT, verify that no new child member
+      // is present.
+      if ((replacedDefAtom->contentType() == DefinedAtom::typeGroupComdat) &&
+          (iterDefAtom->contentType() == DefinedAtom::typeGroupComdat)) {
+        std::vector<std::string> groupMemberDifference;
----------------
This is a lot of checks to see if both atoms are group atoms.  A simpler way is in addGroup() in case of collision, the group not chosen should be added to a new _replacedGroup map.  Then here in removeCoalescedAwayAtoms(),  you first iterate the _replacedGroup and make sure the children are correctly put into _replacedAtoms, then just to what removeCoalescedAwayAtoms() did before.

================
Comment at: lib/Core/SymbolTable.cpp:197-198
@@ +196,4 @@
+            return;
+          if (signatureAtomForExisting->name() == signatureAtomForNew->name())
+            useNew = false;
+          break;
----------------
At this point we may not know which group will be used, so we may want to punt and determine the replacement part later.

================
Comment at: lib/Core/Resolver.cpp:445-458
@@ +444,16 @@
+          (iterDefAtom->contentType() == DefinedAtom::typeGroupComdat)) {
+        std::vector<std::string> groupMemberDifference;
+        if (!verifyGroupMembers(iterDefAtom, replacedDefAtom,
+                                groupMemberDifference)) {
+          llvm::errs() << "New group member exists in " << iterDefAtom->name()
+                       << ":" << iterDefAtom->file().path()
+                       << " which is not found in group "
+                       << replacedDefAtom->name() << ":"
+                       << replacedDefAtom->file().path() << "\n";
+          llvm::errs() << "Group difference (new members)"
+                       << "\n";
+          for (auto mem : groupMemberDifference)
+            llvm::errs() << mem << " ";
+          llvm::errs() << "\n";
+          llvm::report_fatal_error("group resolution error");
+        }
----------------
I don't think we need to compare the members (unless there is some command line option for debugging group comdat coalescing).  You can have files compiled with different optimizations which may lead to different inlining and thus different symbols exposed for the same group.


http://llvm-reviews.chandlerc.com/D2961