[PATCH] sectiongroup support

[Shankar Kalpathi Easwaran]

  Thanks for the review, Nick.


================
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;
----------------
kledzik at apple.com wrote:
> 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.
Yes this would be much simpler.

================
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");
+        }
----------------
kledzik at apple.com wrote:
> 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.
could this be a warning instead of a error to notify the user ?

================
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)
----------------
kledzik at apple.com wrote:
> This needs to handle the case where the childAtom is not a DefinedAtom.  Currently it will crash because definedAtom will be NULL.
I will return NULL, as soon as I dont see a definedAtom. But I wonder if it will ever reach this function with the context of an undef atom. 

This is called under MCR_Error.

================
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())
----------------
kledzik at apple.com wrote:
> 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);
This is a very good catch. I was adding the group child members when I saw the group and this was left over. the assert would be a good catch of this.

================
Comment at: lib/Core/SymbolTable.cpp:197-198
@@ +196,4 @@
+            return;
+          if (signatureAtomForExisting->name() == signatureAtomForNew->name())
+            useNew = false;
+          break;
----------------
kledzik at apple.com wrote:
> At this point we may not know which group will be used, so we may want to punt and determine the replacement part later.
Do we regard the atom as just a duplicate atom, if the signatures dont match and leave it to the replacement to decide atoms in the group to be coalesced ?


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