[PATCH] add support for LayoutAfter/LayoutBefore references

[Michael Spencer]

================
Comment at: include/lld/Core/Pass.h:140-141
@@ +139,4 @@
+
+  typedef std::map<const Atom *, const Atom *> AtomToAtomT;
+  typedef std::map<const Atom *, uint64_t> AtomToOrdinalT;
+  AtomToAtomT _followOnNexts;
----------------
These would be better as llvm::DenseMap.

================
Comment at: include/lld/Core/Pass.h:117-146
@@ -113,11 +116,32 @@
 
-/// Pass for sorting atoms.
-class OrderPass : public Pass {
+/// Pass for atoms layout.
+class LayoutPass : public Pass {
 public:
-  OrderPass() : Pass() {}
-  
+  class CompareAtoms {
+  public:
+    CompareAtoms(const LayoutPass &pass) : _layout(pass) {}
+    bool operator()(const DefinedAtom *left, const DefinedAtom *right);
+  private:
+    const LayoutPass &_layout;
+  };
+
+  LayoutPass() : Pass(), _compareAtoms(*this) {}
+
   /// Sorts atoms in mergedFile by content type then by command line order.
   virtual void perform(MutableFile &mergedFile);
-};
 
+  virtual ~LayoutPass() {}
+
+private:
+  void buildFollowOnTable(MutableFile::DefinedAtomRange &range);
+  void buildPrecededByTable(MutableFile::DefinedAtomRange &range);
+  void buildOrdinalOverrideMap(MutableFile::DefinedAtomRange &range);
+
+  typedef std::map<const Atom *, const Atom *> AtomToAtomT;
+  typedef std::map<const Atom *, uint64_t> AtomToOrdinalT;
+  AtomToAtomT _followOnNexts;
+  AtomToAtomT _followOnRoots;
+  AtomToOrdinalT _ordinalOverrideMap;
+  CompareAtoms _compareAtoms;
+};
 
----------------
This should really be in the Passes library.

================
Comment at: include/lld/Core/Reference.h:52-73
@@ -45,2 +51,24 @@
 
+  virtual StringRef kindToString() const {
+    switch (kind()) {
+    case -1:
+      return "layoutbefore";
+      break;
+    case -2:
+      return "layoutafter";
+      break;
+    default:
+      return "unknown";
+    }
+  }
+
+  virtual int32_t stringToKind(StringRef kindString) const {
+    if (kindString == "layoutbefore")
+      return LayoutBefore;
+    else if (kindString == "layoutafter")
+      return LayoutAfter;
+    assert(0 && "unknown relocation kind");
+    return -1;
+  }
+
   /// If the reference is a fixup in the Atom, then this returns the
----------------
This is adding another way to transform string <-> kind. We should have one place be responsible for this, and have it know that kinds < 0 are reserved for the core.

================
Comment at: lib/Passes/LayoutPass.cpp:9-15
@@ +8,9 @@
+//===----------------------------------------------------------------------===//
+//
+// This linker pass does the layout of the atoms. The pass is done after the 
+// order their .o files were found on the command line, then by order of the
+// atoms (address) in the .o file.  But some atoms have a prefered location
+// in their section (such as pinned to the start or end of the section), so
+// the sort must take that into account too.
+//
+//===----------------------------------------------------------------------===//
----------------
Doxygen comment.

================
Comment at: lib/Passes/LayoutPass.cpp:107-110
@@ +106,6 @@
+        // the root of the targetAtom to the root of the current chain
+        if (_followOnRoots.count(targetAtom) == 0) {
+          _followOnRoots[targetAtom] = _followOnRoots[ai];
+        } else {
+          AtomToAtomT::iterator iter = _followOnRoots.find(targetAtom);
+          // The followon is part of another chain
----------------
This is doing the lookup two times. Just do:

  auto iter = _followOnRoots.find(targetAtom);
  if (iter == _followOnRoots.end())
    _followOnRoots[targetAtom] = _followOnRoots[ai];
  else

This is still two lookups in the not-found case. You could actually reduce this to one lookup with lower_bound, but that doesn't work if you switch to DenseMap.

================
Comment at: lib/Passes/LayoutPass.cpp:93
@@ +92,3 @@
+}
+
+void LayoutPass::buildFollowOnTable(MutableFile::DefinedAtomRange &range) {
----------------
Can we get an overview of this algorithm?

================
Comment at: lib/Passes/LayoutPass.cpp:111-112
@@ +110,4 @@
+          AtomToAtomT::iterator iter = _followOnRoots.find(targetAtom);
+          // The followon is part of another chain
+          if (iter->second == targetAtom) {
+            const Atom *a = targetAtom;
----------------
The comment doesn't seem to match the condition. Do you mean the target is the start of another chain?

================
Comment at: lib/Passes/LayoutPass.cpp:115
@@ +114,3 @@
+            while (true) {
+              _followOnRoots[a] = _followOnRoots[ai];
+              // Set all the follow on's for the targetAtom to be
----------------
_followOnRoots[ai] was already looked up on line 102. That lookup should be reused here.

================
Comment at: lib/Passes/LayoutPass.cpp:128
@@ +127,3 @@
+                   // Get to the root of the chain
+                   // TODO : fix it 
+            const Atom *a = _followOnRoots[targetAtom];
----------------
Fix what?

================
Comment at: lib/Passes/LayoutPass.cpp:145
@@ +144,3 @@
+
+              if (((DefinedAtom *)a)->size() != 0) {
+                foundNonZeroSizeAtom = true;
----------------
You should just be using DefinedAtom everywhere. Is MutableFile::DefinedAtomRange not a const DefinedAtom *?

================
Comment at: lib/Passes/LayoutPass.cpp:179-180
@@ +178,4 @@
+}
+
+void LayoutPass::buildPrecededByTable(MutableFile::DefinedAtomRange &range) {
+  // This table would convert precededby references to follow on 
----------------
Describe algorithm.

================
Comment at: lib/Passes/LayoutPass.cpp:183-185
@@ +182,5 @@
+  // references so that we have only one table 
+  for (auto ai : range) {
+    for (const Reference *r : *ai) {
+      if (r->kind() == lld::Reference::LayoutBefore) {
+        const Atom *targetAtom = r->target();
----------------
Instead of looping over everything multiple times, it would be better to loop once and dispatch on r->kind(). That is if the algorithm can be made to handle both LayoutAfter and LayoutBefore at the same time.

================
Comment at: lib/Passes/LayoutPass.cpp:200
@@ +199,3 @@
+          }
+        } else if ((_followOnRoots.find(targetAtom))->second == targetAtom) {
+          // Is the targetAtom in chain with the targetAtom as the root ?
----------------
The extra () are unnecessary.


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