As much as I hate to say it, the whole setNode interface for DSNodeHandles

is HOPELESSLY broken.  The problem is that the embedded getNode call can
change the offset of the node handle in unpredictable ways.

As it turns out, all of the clients of this method really want to set
both the node and the offset, thus it is more efficient (and less buggy)
to just do both of them in one method call.  This fixes some obscure bugs
handling non-forwarded node handles.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@14660 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/Analysis/DSNode.h b/include/llvm/Analysis/DSNode.h
index f1bb90ed9c6b1fef25eb89587a8423468e976632..4c6480d0077d98dbb2709fdcceeecf7a261d591c 100644 (file)
--- a/include/llvm/Analysis/DSNode.h
+++ b/include/llvm/Analysis/DSNode.h
@@ -175,7 +175,7 @@ public:
   void stopForwarding() {
     assert(isForwarding() &&
            "Node isn't forwarding, cannot stopForwarding()!");
-    ForwardNH.setNode(0);
+    ForwardNH.setTo(0, 0);
     assert(ParentGraph == 0 &&
            "Forwarding nodes must have been removed from graph!");
     delete this;
@@ -336,7 +336,7 @@ public:
   void dropAllReferences() {
     Links.clear();
     if (isForwarding())
-      ForwardNH.setNode(0);
+      ForwardNH.setTo(0, 0);
   }
 
   /// remapLinks - Change all of the Links in the current node according to the
@@ -401,10 +401,11 @@ inline DSNode *DSNodeHandle::getNode() const {
   return HandleForwarding();
 }
 
-inline void DSNodeHandle::setNode(DSNode *n) const {
+inline void DSNodeHandle::setTo(DSNode *n, unsigned NewOffset) const {
   assert(!n || !n->isForwarding() && "Cannot set node to a forwarded node!");
   if (N) getNode()->NumReferrers--;
   N = n;
+  Offset = NewOffset;
   if (N) {
     N->NumReferrers++;
     if (Offset >= N->Size) {
@@ -457,8 +458,8 @@ inline void DSNodeHandle::mergeWith(const DSNodeHandle &Node) const {
     getNode()->mergeWith(Node, Offset);
   else {   // No node to merge with, so just point to Node
     Offset = 0;
-    setNode(Node.getNode());
-    Offset = Node.getOffset();
+    DSNode *NN = Node.getNode();
+    setTo(NN, Node.getOffset());
   }
 }
 

diff --git a/include/llvm/Analysis/DSSupport.h b/include/llvm/Analysis/DSSupport.h
index fdbb43236ee51bb17c3bf0790f7a9b63c4a64975..8cce6c98fde8fa8f41157e5a6b9877984862a37c 100644 (file)
--- a/include/llvm/Analysis/DSSupport.h
+++ b/include/llvm/Analysis/DSSupport.h
@@ -58,17 +58,18 @@ class DSNodeHandle {
   void operator==(const DSNode *N);  // DISALLOW, use to promote N to nodehandle
 public:
   // Allow construction, destruction, and assignment...
-  DSNodeHandle(DSNode *n = 0, unsigned offs = 0) : N(0), Offset(offs) {
-    setNode(n);
+  DSNodeHandle(DSNode *n = 0, unsigned offs = 0) : N(0), Offset(0) {
+    setTo(n, offs);
   }
   DSNodeHandle(const DSNodeHandle &H) : N(0), Offset(0) {
-    setNode(H.getNode());
-    Offset = H.Offset;      // Must read offset AFTER the getNode()
+    DSNode *NN = H.getNode();
+    setTo(NN, H.Offset);  // Must read offset AFTER the getNode()
   }
-  ~DSNodeHandle() { setNode((DSNode*)0); }
+  ~DSNodeHandle() { setTo(0, 0); }
   DSNodeHandle &operator=(const DSNodeHandle &H) {
     if (&H == this) return *this;  // Don't set offset to 0 if self assigning.
-    Offset = 0; setNode(H.getNode()); Offset = H.Offset;
+    DSNode *NN = H.getNode();  // Call getNode() before .Offset
+    setTo(NN, H.Offset);
     return *this;
   }
 
@@ -96,7 +97,6 @@ public:
   inline DSNode *getNode() const;  // Defined inline in DSNode.h
   unsigned getOffset() const { return Offset; }
 
-  inline void setNode(DSNode *N) const;  // Defined inline in DSNode.h
   void setOffset(unsigned O) {
     //assert((!N || Offset < N->Size || (N->Size == 0 && Offset == 0) ||
     //       !N->ForwardNH.isNull()) && "Node handle offset out of range!");
@@ -105,6 +105,8 @@ public:
     Offset = O;
   }
 
+  inline void setTo(DSNode *N, unsigned O) const; // Defined inline in DSNode.h
+
   void addEdgeTo(unsigned LinkNo, const DSNodeHandle &N);
   void addEdgeTo(const DSNodeHandle &N) { addEdgeTo(0, N); }
 
@@ -154,9 +156,7 @@ class DSCallSite {
     if (DSNode *N = Src.getNode()) {
       hash_map<const DSNode*, DSNode*>::const_iterator I = NodeMap.find(N);
       assert(I != NodeMap.end() && "Node not in mapping!");
-
-      NH.setOffset(Src.getOffset());
-      NH.setNode(I->second);
+      NH.setTo(I->second, Src.getOffset());
     }
   }
 
@@ -166,8 +166,8 @@ class DSCallSite {
       hash_map<const DSNode*, DSNodeHandle>::const_iterator I = NodeMap.find(N);
       assert(I != NodeMap.end() && "Node not in mapping!");
 
-      NH.setOffset(Src.getOffset()+I->second.getOffset());
-      NH.setNode(I->second.getNode());
+      DSNode *NN = I->second.getNode(); // Call getNode before getOffset()
+      NH.setTo(NN, Src.getOffset()+I->second.getOffset());
     }
   }
 

diff --git a/include/llvm/Analysis/DataStructure/DSNode.h b/include/llvm/Analysis/DataStructure/DSNode.h
index f1bb90ed9c6b1fef25eb89587a8423468e976632..4c6480d0077d98dbb2709fdcceeecf7a261d591c 100644 (file)
--- a/include/llvm/Analysis/DataStructure/DSNode.h
+++ b/include/llvm/Analysis/DataStructure/DSNode.h
@@ -175,7 +175,7 @@ public:
   void stopForwarding() {
     assert(isForwarding() &&
            "Node isn't forwarding, cannot stopForwarding()!");
-    ForwardNH.setNode(0);
+    ForwardNH.setTo(0, 0);
     assert(ParentGraph == 0 &&
            "Forwarding nodes must have been removed from graph!");
     delete this;
@@ -336,7 +336,7 @@ public:
   void dropAllReferences() {
     Links.clear();
     if (isForwarding())
-      ForwardNH.setNode(0);
+      ForwardNH.setTo(0, 0);
   }
 
   /// remapLinks - Change all of the Links in the current node according to the
@@ -401,10 +401,11 @@ inline DSNode *DSNodeHandle::getNode() const {
   return HandleForwarding();
 }
 
-inline void DSNodeHandle::setNode(DSNode *n) const {
+inline void DSNodeHandle::setTo(DSNode *n, unsigned NewOffset) const {
   assert(!n || !n->isForwarding() && "Cannot set node to a forwarded node!");
   if (N) getNode()->NumReferrers--;
   N = n;
+  Offset = NewOffset;
   if (N) {
     N->NumReferrers++;
     if (Offset >= N->Size) {
@@ -457,8 +458,8 @@ inline void DSNodeHandle::mergeWith(const DSNodeHandle &Node) const {
     getNode()->mergeWith(Node, Offset);
   else {   // No node to merge with, so just point to Node
     Offset = 0;
-    setNode(Node.getNode());
-    Offset = Node.getOffset();
+    DSNode *NN = Node.getNode();
+    setTo(NN, Node.getOffset());
   }
 }
 

diff --git a/include/llvm/Analysis/DataStructure/DSSupport.h b/include/llvm/Analysis/DataStructure/DSSupport.h
index fdbb43236ee51bb17c3bf0790f7a9b63c4a64975..8cce6c98fde8fa8f41157e5a6b9877984862a37c 100644 (file)
--- a/include/llvm/Analysis/DataStructure/DSSupport.h
+++ b/include/llvm/Analysis/DataStructure/DSSupport.h
@@ -58,17 +58,18 @@ class DSNodeHandle {
   void operator==(const DSNode *N);  // DISALLOW, use to promote N to nodehandle
 public:
   // Allow construction, destruction, and assignment...
-  DSNodeHandle(DSNode *n = 0, unsigned offs = 0) : N(0), Offset(offs) {
-    setNode(n);
+  DSNodeHandle(DSNode *n = 0, unsigned offs = 0) : N(0), Offset(0) {
+    setTo(n, offs);
   }
   DSNodeHandle(const DSNodeHandle &H) : N(0), Offset(0) {
-    setNode(H.getNode());
-    Offset = H.Offset;      // Must read offset AFTER the getNode()
+    DSNode *NN = H.getNode();
+    setTo(NN, H.Offset);  // Must read offset AFTER the getNode()
   }
-  ~DSNodeHandle() { setNode((DSNode*)0); }
+  ~DSNodeHandle() { setTo(0, 0); }
   DSNodeHandle &operator=(const DSNodeHandle &H) {
     if (&H == this) return *this;  // Don't set offset to 0 if self assigning.
-    Offset = 0; setNode(H.getNode()); Offset = H.Offset;
+    DSNode *NN = H.getNode();  // Call getNode() before .Offset
+    setTo(NN, H.Offset);
     return *this;
   }
 
@@ -96,7 +97,6 @@ public:
   inline DSNode *getNode() const;  // Defined inline in DSNode.h
   unsigned getOffset() const { return Offset; }
 
-  inline void setNode(DSNode *N) const;  // Defined inline in DSNode.h
   void setOffset(unsigned O) {
     //assert((!N || Offset < N->Size || (N->Size == 0 && Offset == 0) ||
     //       !N->ForwardNH.isNull()) && "Node handle offset out of range!");
@@ -105,6 +105,8 @@ public:
     Offset = O;
   }
 
+  inline void setTo(DSNode *N, unsigned O) const; // Defined inline in DSNode.h
+
   void addEdgeTo(unsigned LinkNo, const DSNodeHandle &N);
   void addEdgeTo(const DSNodeHandle &N) { addEdgeTo(0, N); }
 
@@ -154,9 +156,7 @@ class DSCallSite {
     if (DSNode *N = Src.getNode()) {
       hash_map<const DSNode*, DSNode*>::const_iterator I = NodeMap.find(N);
       assert(I != NodeMap.end() && "Node not in mapping!");
-
-      NH.setOffset(Src.getOffset());
-      NH.setNode(I->second);
+      NH.setTo(I->second, Src.getOffset());
     }
   }
 
@@ -166,8 +166,8 @@ class DSCallSite {
       hash_map<const DSNode*, DSNodeHandle>::const_iterator I = NodeMap.find(N);
       assert(I != NodeMap.end() && "Node not in mapping!");
 
-      NH.setOffset(Src.getOffset()+I->second.getOffset());
-      NH.setNode(I->second.getNode());
+      DSNode *NN = I->second.getNode(); // Call getNode before getOffset()
+      NH.setTo(NN, Src.getOffset()+I->second.getOffset());
     }
   }
 

diff --git a/lib/Analysis/DataStructure/DataStructure.cpp b/lib/Analysis/DataStructure/DataStructure.cpp
index 53c39f9047cc958b3d81a810639729a9f249797d..7b0ddba8055f12e3897f69977c7d9a6b787422d2 100644 (file)
--- a/lib/Analysis/DataStructure/DataStructure.cpp
+++ b/lib/Analysis/DataStructure/DataStructure.cpp
@@ -120,8 +120,7 @@ void DSNode::forwardNode(DSNode *To, unsigned Offset) {
   if (To->Size <= 1) Offset = 0;
   assert((Offset < To->Size || (Offset == To->Size && Offset == 0)) &&
          "Forwarded offset is wrong!");
-  ForwardNH.setNode(To);
-  ForwardNH.setOffset(Offset);
+  ForwardNH.setTo(To, Offset);
   NodeType = DEAD;
   Size = 0;
   Ty = Type::VoidTy;
@@ -1096,10 +1095,9 @@ void DSNode::remapLinks(DSGraph::NodeMapTy &OldNodeMap) {
   for (unsigned i = 0, e = Links.size(); i != e; ++i)
     if (DSNode *N = Links[i].getNode()) {
       DSGraph::NodeMapTy::const_iterator ONMI = OldNodeMap.find(N);
-      if (ONMI != OldNodeMap.end()) {
-        Links[i].setNode(ONMI->second.getNode());
-        Links[i].setOffset(Links[i].getOffset()+ONMI->second.getOffset());
-      }
+      if (ONMI != OldNodeMap.end())
+        Links[i].setTo(ONMI->second.getNode(),
+                       Links[i].getOffset()+ONMI->second.getOffset());
     }
 }
 
@@ -1475,7 +1473,7 @@ static inline void killIfUselessEdge(DSNodeHandle &Edge) {
       // No interesting info?
       if ((N->getNodeFlags() & ~DSNode::Incomplete) == 0 &&
           N->getType() == Type::VoidTy && !N->isNodeCompletelyFolded())
-        Edge.setNode(0);  // Kill the edge!
+        Edge.setTo(0, 0);  // Kill the edge!
 }
 
 static inline bool nodeContainsExternalFunction(const DSNode *N) {
@@ -1979,8 +1977,7 @@ void DSGraph::computeNodeMapping(const DSNodeHandle &NH1,
     return;
   }
   
-  Entry.setNode(N2);
-  Entry.setOffset(NH2.getOffset()-NH1.getOffset());
+  Entry.setTo(N2, NH2.getOffset()-NH1.getOffset());
 
   // Loop over all of the fields that N1 and N2 have in common, recursively
   // mapping the edges together now.

diff --git a/lib/Analysis/DataStructure/Local.cpp b/lib/Analysis/DataStructure/Local.cpp
index 4a61e8bb1068b71d759607a502c5d2a9f84f55a6..62dc7af2f16b20ec011042655b338be5f96d627c 100644 (file)
--- a/lib/Analysis/DataStructure/Local.cpp
+++ b/lib/Analysis/DataStructure/Local.cpp
@@ -259,8 +259,7 @@ DSNodeHandle GraphBuilder::getValueDest(Value &Val) {
     N = createNode();
   }
 
-  NH.setNode(N);      // Remember that we are pointing to it...
-  NH.setOffset(0);
+  NH.setTo(N, 0);      // Remember that we are pointing to it...
   return NH;
 }