[PATCH] [LayoutPass] Add a method to check the followon graph structure.

[Shankar Kalpathi Easwaran]

  You should try adding a test too, by mixing Debug outputs too. Wait for BigCheese to review this too.


================
Comment at: include/lld/Core/Atom.h:68-73
@@ -67,2 +67,8 @@
 
+  // Returns a human-readable string representation of this atom. Used
+  // only for debugging.
+  virtual std::string toDebugString() const {
+    return name().empty() ? "<anonymous>" : name();
+  }
+
 protected:
----------------
This should be moved the code thats using it.

================
Comment at: lib/Passes/LayoutPass.cpp:412-415
@@ +411,6 @@
+      std::string refstr = ref->target()->toDebugString();
+      if (ref->kind() == lld::Reference::kindLayoutBefore) {
+        llvm::dbgs () << "    layout-before: " << refstr << "\n";
+      } else if (ref->kind() == lld::Reference::kindLayoutAfter) {
+        llvm::dbgs () << "    layout-after: " << refstr << "\n";
+      }
----------------
not handling In-Group references.

================
Comment at: lib/Passes/LayoutPass.cpp:423-425
@@ +422,5 @@
+
+// Exit if there's a cycle in a followon chain reachable from the
+// given root atom. Uses the tortoise and hare algorithm to detect a
+// cycle.
+void checkNoCycleInFollowonChain(AtomToAtomT &followOnNexts,
----------------
doxygen ?

================
Comment at: lib/Passes/LayoutPass.cpp:426-438
@@ +425,15 @@
+// cycle.
+void checkNoCycleInFollowonChain(AtomToAtomT &followOnNexts,
+                                 const DefinedAtom *root) {
+  const DefinedAtom *tortoise = root;
+  const DefinedAtom *hare = followOnNexts[root];
+  while (true) {
+    if (!tortoise || !hare)
+      return;
+    if (tortoise == hare)
+      showCycleDetectedError(followOnNexts, tortoise);
+    tortoise = followOnNexts[tortoise];
+    hare = followOnNexts[followOnNexts[hare]];
+  }
+}
+
----------------
can all these functions be const ?

================
Comment at: lib/Passes/LayoutPass.cpp:440-461
@@ +439,24 @@
+
+void checkReachabilityFromRoot(AtomToAtomT &followOnRoots,
+                               const DefinedAtom *atom) {
+  if (!atom) return;
+  auto i = followOnRoots.find(atom);
+  if (i == followOnRoots.end()) {
+    std::string msg = "Atom <" + atom->toDebugString()
+        + "> has no follow-on root!";
+    llvm_unreachable(msg.c_str());
+  }
+  const DefinedAtom *ap = i->second;
+  while (true) {
+    const DefinedAtom *next = followOnRoots[ap];
+    if (!next) {
+      std::string msg = "Atom <" + atom->toDebugString()
+          + "> is not reachable from its root!";
+      llvm_unreachable(msg.c_str());
+    }
+    if (next == ap)
+      return;
+    ap = next;
+  }
+}
+
----------------
const function ?

================
Comment at: lib/Passes/LayoutPass.cpp:463-472
@@ +462,12 @@
+
+void printDefinedAtoms(const MutableFile::DefinedAtomRange &atomRange) {
+  for (const DefinedAtom *atom : atomRange) {
+    llvm::dbgs() << "  file=" << atom->file().path()
+                 << ", name=" << atom->name()
+                 << ", size=" << atom->size()
+                 << ", type=" << atom->contentType()
+                 << ", ordinal=" << atom->ordinal()
+                 << "\n";
+  }
+}
+} // end anonymous namespace
----------------
const function ?

================
Comment at: lib/Passes/LayoutPass.cpp:475-493
@@ +474,21 @@
+
+/// Verify that the followon chain is sane. Should not be called in
+/// release binary.
+void LayoutPass::checkFollowonChain(MutableFile::DefinedAtomRange &range) {
+  ScopedTask task(getDefaultDomain(), "LayoutPass::checkFollowonChain");
+
+  // Verify that there's no cycle in follow-on chain.
+  std::set<const DefinedAtom *> roots;
+  for (const auto &ai : _followOnRoots)
+    roots.insert(ai.second);
+  for (const DefinedAtom *root : roots)
+    checkNoCycleInFollowonChain(_followOnNexts, root);
+
+  // Verify that all the atoms in followOnNexts have references to
+  // their roots.
+  for (const auto &ai : _followOnNexts) {
+    checkReachabilityFromRoot(_followOnRoots, ai.first);
+    checkReachabilityFromRoot(_followOnRoots, ai.second);
+  }
+}
+
----------------
all of the code that shouldnt be in release build should be in 

ifndef NDEBUG

================
Comment at: lib/ReaderWriter/ELF/Atoms.h:484-488
@@ -483,2 +483,7 @@
 
+  std::string toDebugString() const {
+    std::string name = DefinedAtom::toDebugString();
+    return name + " in " + _sectionName.str();
+  }
+
 private:
----------------
should be moved to LayoutPass, you can call toDebugString(Atom) and have this function in the LayoutPass.



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