[llvm-commits] CVS: llvm/include/llvm/Analysis/DSNode.h DSSupport.h

[Chris Lattner]

Changes in directory llvm/include/llvm/Analysis:

DSNode.h updated: 1.42 -> 1.43
DSSupport.h updated: 1.31 -> 1.32

---
Log message:

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.



---
Diffs of the changes:  (+18 -17)

Index: llvm/include/llvm/Analysis/DSNode.h
diff -u llvm/include/llvm/Analysis/DSNode.h:1.42 llvm/include/llvm/Analysis/DSNode.h:1.43
--- llvm/include/llvm/Analysis/DSNode.h:1.42	Sun May 23 02:34:53 2004
+++ llvm/include/llvm/Analysis/DSNode.h	Wed Jul  7 01:12:36 2004
@@ -175,7 +175,7 @@
   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 @@
   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 @@
   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 @@
     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());
   }
 }
 


Index: llvm/include/llvm/Analysis/DSSupport.h
diff -u llvm/include/llvm/Analysis/DSSupport.h:1.31 llvm/include/llvm/Analysis/DSSupport.h:1.32
--- llvm/include/llvm/Analysis/DSSupport.h:1.31	Thu Mar 11 17:08:20 2004
+++ llvm/include/llvm/Analysis/DSSupport.h	Wed Jul  7 01:12:36 2004
@@ -58,17 +58,18 @@
   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 @@
   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 @@
     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 @@
     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 @@
       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());
     }
   }