This fixes 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. :)


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

2 files changed, 50 insertions, 4 deletions

diff --git a/lib/VMCore/SymbolTable.cpp b/lib/VMCore/SymbolTable.cpp
index d85ceed..c38b8e0 100644
--- a/lib/VMCore/SymbolTable.cpp
+++ b/lib/VMCore/SymbolTable.cpp
@@ -102,6 +102,42 @@ void SymbolTable::remove(Value *N) {
   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) {
diff --git a/lib/VMCore/Value.cpp b/lib/VMCore/Value.cpp
index cdde96b..dd06441 100644
--- a/lib/VMCore/Value.cpp
+++ b/lib/VMCore/Value.cpp
@@ -96,8 +96,8 @@ unsigned Value::getNumUses() const {
 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 @@ void Value::setName(const std::string &name) {
     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,