[PATCH] add support for LayoutAfter/LayoutBefore references

[Shankar Kalpathi Easwaran]

================
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;
+};
 
----------------
Michael Spencer wrote:
> This should really be in the Passes library.
These are member variables that need to be accessed by all the functions, I am not sure what you meant by the comment here.

================
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;
----------------
Michael Spencer wrote:
> These would be better as llvm::DenseMap.
Ok.

================
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
----------------
Michael Spencer wrote:
> 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.
Thats why its in Reference.

================
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.
+//
+//===----------------------------------------------------------------------===//
----------------
Michael Spencer wrote:
> Doxygen comment.
Ok.

================
Comment at: lib/Passes/LayoutPass.cpp:93
@@ +92,3 @@
+}
+
+void LayoutPass::buildFollowOnTable(MutableFile::DefinedAtomRange &range) {
----------------
Michael Spencer wrote:
> Can we get an overview of this algorithm?
I added only ordinaloverride map and the other came from order pass, I will document anyways.

================
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
----------------
Michael Spencer wrote:
> 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.
Ok.

================
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;
----------------
Michael Spencer wrote:
> The comment doesn't seem to match the condition. Do you mean the target is the start of another chain?
Yes. will fix the comment.

================
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
----------------
Michael Spencer wrote:
> _followOnRoots[ai] was already looked up on line 102. That lookup should be reused here.
Ok.

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

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

================
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 
----------------
Michael Spencer wrote:
> Describe algorithm.
Ok, will do.

================
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();
----------------
Michael Spencer wrote:
> 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.
We need to collect all the follow on references before moving to precededby.

================
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 ?
----------------
Michael Spencer wrote:
> The extra () are unnecessary.
Ok.


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