Fix bug that CompareAtoms::compare is not transitive.

This patch fixes a bug in r190608. The results of a comparison function
passed to std::sort must be transitive, which is, if a < b and b < c, and if
a != b, a < c must be also true. CompareAtoms::compare did not actually
guarantee the transitivity. As a result the sort results were sometimes just
wrong.

Consider there are three atoms, X, Y, and Z, whose file ordinals are 1, 2, 3,
respectively. Z has a property "layout-after X". In this case, all the
following conditionals become true:

  X < Y because X's ordinal is less than Y's
  Y < Z because Y's ordinal is less than Z's
  Z < X because of the layout-after relationship

This is not of course transitive. The reason why this happened is because
we used follow-on relationships for comparison if two atoms falls in the same
follow-on chain, but we used each atom's properties if they did not. This patch
fixes the issue by using follow-on root atoms for comparison to get consistent
results.

Differential Revision: http://llvm-reviews.chandlerc.com/D1980

llvm-svn: 193029

lld/include/lld/Passes/LayoutPass.h

@@ -68,11 +68,6 @@ private:
   // Build a map of Atoms to ordinals for sorting the atoms
   void buildOrdinalOverrideMap(MutableFile::DefinedAtomRange &range);
 
-#ifndef NDEBUG
-  // Check if the follow-on graph is a correct structure. For debugging only.
-  void checkFollowonChain(MutableFile::DefinedAtomRange &range);
-#endif
-
   typedef llvm::DenseMap<const DefinedAtom *, const DefinedAtom *> AtomToAtomT;
   typedef llvm::DenseMap<const DefinedAtom *, uint64_t> AtomToOrdinalT;
 
@@ -96,6 +91,14 @@ private:
   bool checkAllPrevAtomsZeroSize(const DefinedAtom *targetAtom);
 
   void setChainRoot(const DefinedAtom *targetAtom, const DefinedAtom *root);
+
+#ifndef NDEBUG
+  // Check if the follow-on graph is a correct structure. For debugging only.
+  void checkFollowonChain(MutableFile::DefinedAtomRange &range);
+
+  typedef std::vector<const DefinedAtom *>::iterator DefinedAtomIter;
+  void checkTransitivity(DefinedAtomIter begin, DefinedAtomIter end) const;
+#endif
 };
 
 } // namespace lld

lld/lib/Passes/LayoutPass.cpp

@@ -28,6 +28,19 @@ std::string formatReason(StringRef reason, int leftVal, int rightVal) {
       Twine(reason) + " (" + Twine(leftVal) + ", " + Twine(rightVal) + ")";
   return std::move(msg.str());
 }
+} // end anonymous namespace
+
+// Less-than relationship of two atoms must be transitive, which is, if a < b
+// and b < c, a < c must be true. This function checks the transitivity by
+// checking the sort results.
+void LayoutPass::checkTransitivity(DefinedAtomIter begin,
+                                   DefinedAtomIter end) const {
+  for (DefinedAtomIter i = begin; (i + 1) != end; ++i) {
+    for (DefinedAtomIter j = i + 1; j != end; ++j) {
+      assert(_compareAtoms(*i, *j));
+      assert(!_compareAtoms(*j, *i));
+    }
+  }
 }
 #endif // NDEBUG
 
@@ -60,27 +73,27 @@ bool LayoutPass::CompareAtoms::compare(const DefinedAtom *left,
     }
   }
 
-  AtomToOrdinalT::const_iterator lPos = _layout._ordinalOverrideMap.find(left);
-  AtomToOrdinalT::const_iterator rPos = _layout._ordinalOverrideMap.find(right);
-  AtomToOrdinalT::const_iterator end = _layout._ordinalOverrideMap.end();
+  // Find the root of the chain if it is a part of a follow-on chain.
+  auto leftFind = _layout._followOnRoots.find(left);
+  auto rightFind = _layout._followOnRoots.find(right);
+  const DefinedAtom *leftRoot =
+      (leftFind == _layout._followOnRoots.end()) ? left : leftFind->second;
+  const DefinedAtom *rightRoot =
+      (rightFind == _layout._followOnRoots.end()) ? right : rightFind->second;
 
   // Sort atoms by their ordinal overrides only if they fall in the same
   // chain.
-  auto leftAtom = _layout._followOnRoots.find(left);
-  auto rightAtom = _layout._followOnRoots.find(right);
-
-  if (leftAtom != _layout._followOnRoots.end() &&
-      rightAtom != _layout._followOnRoots.end() &&
-      leftAtom->second == rightAtom->second) {
-    if ((lPos != end) && (rPos != end)) {
-      DEBUG(reason = formatReason("override", lPos->second, rPos->second));
-      return lPos->second < rPos->second;
-    }
+  AtomToOrdinalT::const_iterator lPos = _layout._ordinalOverrideMap.find(left);
+  AtomToOrdinalT::const_iterator rPos = _layout._ordinalOverrideMap.find(right);
+  AtomToOrdinalT::const_iterator end = _layout._ordinalOverrideMap.end();
+  if (leftRoot == rightRoot && lPos != end && rPos != end) {
+    DEBUG(reason = formatReason("override", lPos->second, rPos->second));
+    return lPos->second < rPos->second;
   }
 
   // Sort same permissions together.
-  DefinedAtom::ContentPermissions leftPerms = left->permissions();
-  DefinedAtom::ContentPermissions rightPerms = right->permissions();
+  DefinedAtom::ContentPermissions leftPerms = leftRoot->permissions();
+  DefinedAtom::ContentPermissions rightPerms = rightRoot->permissions();
 
   if (leftPerms != rightPerms) {
     DEBUG(reason =
@@ -89,8 +102,8 @@ bool LayoutPass::CompareAtoms::compare(const DefinedAtom *left,
   }
 
   // Sort same content types together.
-  DefinedAtom::ContentType leftType = left->contentType();
-  DefinedAtom::ContentType rightType = right->contentType();
+  DefinedAtom::ContentType leftType = leftRoot->contentType();
+  DefinedAtom::ContentType rightType = rightRoot->contentType();
 
   if (leftType != rightType) {
     DEBUG(reason = formatReason("contentType", (int)leftType, (int)rightType));
@@ -98,8 +111,8 @@ bool LayoutPass::CompareAtoms::compare(const DefinedAtom *left,
   }
 
   // Sort by .o order.
-  const File *leftFile = &left->file();
-  const File *rightFile = &right->file();
+  const File *leftFile = &leftRoot->file();
+  const File *rightFile = &rightRoot->file();
 
   if (leftFile != rightFile) {
     DEBUG(reason = formatReason(".o order", (int)leftFile->ordinal(),
@@ -108,12 +121,12 @@ bool LayoutPass::CompareAtoms::compare(const DefinedAtom *left,
   }
 
   // Sort by atom order with .o file.
-  uint64_t leftOrdinal = left->ordinal();
-  uint64_t rightOrdinal = right->ordinal();
+  uint64_t leftOrdinal = leftRoot->ordinal();
+  uint64_t rightOrdinal = rightRoot->ordinal();
 
   if (leftOrdinal != rightOrdinal) {
-    DEBUG(reason = formatReason("ordinal", (int)left->ordinal(),
-                                (int)right->ordinal()));
+    DEBUG(reason = formatReason("ordinal", (int)leftRoot->ordinal(),
+                                (int)rightRoot->ordinal()));
     return leftOrdinal < rightOrdinal;
   }
 
@@ -548,6 +561,8 @@ void LayoutPass::perform(MutableFile &mergedFile) {
   // sort the atoms
   std::sort(atomRange.begin(), atomRange.end(), _compareAtoms);
 
+  DEBUG(checkTransitivity(atomRange.begin(), atomRange.end()));
+
   DEBUG({
     llvm::dbgs() << "sorted atoms:\n";
     printDefinedAtoms(atomRange);

lld/test/core/layout-transitivity.objtxt

@@ -0,0 +1,33 @@
+# RUN: lld -core --add-pass layout -mllvm -debug %s 2> /dev/null | FileCheck %s
+
+---
+defined-atoms:
+  - name:            fn3
+    scope:           global
+  - name:            fn2
+    scope:           global
+    references:
+      - kind:            layout-after
+        offset:          0
+        target:          fn3
+  - name:            fn
+    scope:           global
+    references:
+      - kind:            layout-after
+        offset:          0
+        target:          fn1
+  - name:            fn4
+    scope:           global
+  - name:            fn1
+    scope:           global
+    references:
+      - kind:            layout-after
+        offset:          0
+        target:          fn2
+...
+
+# CHECK:   - name:            fn
+# CHECK:   - name:            fn1
+# CHECK:   - name:            fn2
+# CHECK:   - name:            fn3
+# CHECK:   - name:            fn4