[llvm-commits] CVS: llvm/lib/VMCore/SymbolTable.cpp Value.cpp

[Chris Lattner]

Changes in directory llvm/lib/VMCore:

SymbolTable.cpp updated: 1.51 -> 1.52
Value.cpp updated: 1.55 -> 1.56
---
Log message:

This fixes PR531: http://llvm.cs.uiuc.edu/PR531 , a crash when running the CBE on a bytecode file.

The problem is that Function::renameLocalSymbols is iterating through
the symbol table planes, occasionally calling setName to rename a value
(which used to do a symbol table remove/insert pair).

The problem is that if there is only a single value in a particular type
plane that the remove will nuke the symbol table plane, and the insert
will create and insert a new one.  This hoses Function::renameLocalSymbols
because it has an iterator to the old plane, under the (very reasonable)
assumption that simply renaming a value won't cause the type plane to 
disappear.

This patch fixes the bug by making the rename operation a single atomic
operation, which has a side effect of making the whole thing faster too. :)



---
Diffs of the changes:  (+50 -4)

 SymbolTable.cpp |   36 ++++++++++++++++++++++++++++++++++++
 Value.cpp       |   18 ++++++++++++++----
 2 files changed, 50 insertions(+), 4 deletions(-)


Index: llvm/lib/VMCore/SymbolTable.cpp
diff -u llvm/lib/VMCore/SymbolTable.cpp:1.51 llvm/lib/VMCore/SymbolTable.cpp:1.52
--- llvm/lib/VMCore/SymbolTable.cpp:1.51	Sat Mar  5 13:02:15 2005
+++ llvm/lib/VMCore/SymbolTable.cpp	Sat Mar  5 20:14:28 2005
@@ -102,6 +102,42 @@
   removeEntry(PI, PI->second.find(N->getName()));
 }
 
+/// changeName - Given a value with a non-empty name, remove its existing entry
+/// from the symbol table and insert a new one for Name.  This is equivalent to
+/// doing "remove(V), V->Name = Name, insert(V)", but is faster, and will not
+/// temporarily remove the symbol table plane if V is the last value in the
+/// symtab with that name (which could invalidate iterators to that plane).
+void SymbolTable::changeName(Value *V, const std::string &name) {
+  assert(!V->getName().empty() && !name.empty() && V->getName() != name &&
+         "Illegal use of this method!");
+
+  plane_iterator PI = pmap.find(V->getType());
+  assert(PI != pmap.end() && "Value doesn't have an entry in this table?");
+  ValueMap &VM = PI->second;
+
+  value_iterator VI;
+
+  if (!InternallyInconsistent) {
+    VI = VM.find(V->getName());
+    assert(VI != VM.end() && "Value does have an entry in this table?");
+
+    // Remove the old entry.
+    VM.erase(VI);
+  }
+
+  // See if we can insert the new name.
+  VI = VM.lower_bound(name);
+
+  // Is there a naming conflict?
+  if (VI != VM.end() && VI->first == name) {
+    V->Name = getUniqueName(V->getType(), name);
+    VM.insert(make_pair(V->Name, V));
+  } else {
+    V->Name = name;
+    VM.insert(VI, make_pair(name, V));
+  }
+}
+
 
 // removeEntry - Remove a value from the symbol table...
 Value *SymbolTable::removeEntry(plane_iterator Plane, value_iterator Entry) {


Index: llvm/lib/VMCore/Value.cpp
diff -u llvm/lib/VMCore/Value.cpp:1.55 llvm/lib/VMCore/Value.cpp:1.56
--- llvm/lib/VMCore/Value.cpp:1.55	Sat Mar  5 13:51:50 2005
+++ llvm/lib/VMCore/Value.cpp	Sat Mar  5 20:14:28 2005
@@ -96,8 +96,8 @@
 void Value::setName(const std::string &name) {
   if (Name == name) return;   // Name is already set.
 
+  // Get the symbol table to update for this object.
   SymbolTable *ST = 0;
-
   if (Instruction *I = dyn_cast<Instruction>(this)) {
     if (BasicBlock *P = I->getParent())
       if (Function *PP = P->getParent())
@@ -113,9 +113,19 @@
     return;  // no name is setable for this.
   }
 
-  if (ST && hasName()) ST->remove(this);
-  Name = name;
-  if (ST && hasName()) ST->insert(this);
+  if (!ST)  // No symbol table to update?  Just do the change.
+    Name = name;
+  else if (hasName()) {
+    if (!name.empty()) {    // Replacing name.
+      ST->changeName(this, name);
+    } else {                // Transitioning from hasName -> noname.
+      ST->remove(this);
+      Name.clear();
+    }
+  } else {                  // Transitioning from noname -> hasName.
+    Name = name;
+    ST->insert(this);
+  }
 }
 
 // uncheckedReplaceAllUsesWith - This is exactly the same as replaceAllUsesWith,