[IR] Allow destruction of symbol table entries regardless of DiscardValueNames

Value::setNameImpl() is used both to set and reset name of the value.
In destructor of Function all arguments get reset their names
(see Function::clearArguments()). If the arguments had their names set (e.g.
when the function was created with LLVMContex::DiscardValueNames == true)
then their ValueName entries referred by the function's symbol table must be
destructed. They are not destructed if LLVMContex::DiscardValueNames is set to
false because of the fast path in Value::setNameImpl(). See the new test cases
that demonstrate the problem. Without the fix they both crash in the function's
destructor.

In Value::setNameImpl() this patch narrows down the fast path return for
DiscardValueNames == true to allow destruction of ValueName entries if any.

Reviewed By: efriedma

Differential Revision: https://reviews.llvm.org/D143487

1 files changed, 14 insertions, 12 deletions

diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 281594d..11dc506 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -315,8 +315,12 @@ StringRef Value::getName() const {
 }
 
 void Value::setNameImpl(const Twine &NewName) {
+  bool NeedNewName =
+      !getContext().shouldDiscardValueNames() || isa<GlobalValue>(this);
+
   // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
-  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
+  // and there is no need to delete the old name.
+  if (!NeedNewName && !hasName())
     return;
 
   // Fast path for common IRBuilder case of setName("") when there is no name.
@@ -324,7 +328,7 @@ void Value::setNameImpl(const Twine &NewName) {
     return;
 
   SmallString<256> NameData;
-  StringRef NameRef = NewName.toStringRef(NameData);
+  StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";
   assert(NameRef.find_first_of(0) == StringRef::npos &&
          "Null bytes are not allowed in names");
 
@@ -340,20 +344,17 @@ void Value::setNameImpl(const Twine &NewName) {
     return;  // Cannot set a name on this value (e.g. constant).
 
   if (!ST) { // No symbol table to update?  Just do the change.
-    if (NameRef.empty()) {
-      // Free the name for this value.
-      destroyValueName();
-      return;
-    }
-
     // NOTE: Could optimize for the case the name is shrinking to not deallocate
     // then reallocated.
     destroyValueName();
 
-    // Create the new name.
-    MallocAllocator Allocator;
-    setValueName(ValueName::create(NameRef, Allocator));
-    getValueName()->setValue(this);
+    if (!NameRef.empty()) {
+      // Create the new name.
+      assert(NeedNewName);
+      MallocAllocator Allocator;
+      setValueName(ValueName::create(NameRef, Allocator));
+      getValueName()->setValue(this);
+    }
     return;
   }
 
@@ -369,6 +370,7 @@ void Value::setNameImpl(const Twine &NewName) {
   }
 
   // Name is changing to something new.
+  assert(NeedNewName);
   setValueName(ST->createValueName(NameRef, this));
 }