[IR] Allow destruction of symbol table entries regardless of DiscardValueNames (Deeper Refactoring)

[yrouban]

Summary

This is a deeper refactoring of D143487 that tries avoiding Twine -> StringData conversion.

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 removes the fast path return for DiscardValueNames == true to allow destruction of ValueName entries.

[yrouban]

removed file permission changes. kept 644.
please review.

[efriedma]

llvm/lib/IR/Value.cpp

324

Is there some reason to move the call to getSymTab() earlier?

334

Why do we want to check isTriviallyEmpty() here? Is this related to the earlier isTriviallyEmpty() check somehow?

344

Not sure why you're messing with this assertion. A void value doesn't have a name, and never had a name (unless you're doing something weird with mutating the types of values, which isn't relevant here).

359

I think the reason the way the code was structured this way is because of this "NOTE"... if you restructure this way, probably delete the NOTE.

[yrouban]

Abandoned this patch in favor of alternative D143487.

llvm/lib/IR/Value.cpp

StringRef Value::getName() const {
   // some clients want to call .data() on the result and expect it to be null
   // terminated.
   if (!hasName())
     return StringRef("", 0);
   return getValueName()->getKey();
 }
 
 void Value::setNameImpl(const Twine &NewName) {
-  // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
-  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
-    return;
-
   // Fast path for common IRBuilder case of setName("") when there is no name.
   if (NewName.isTriviallyEmpty() && !hasName())
     return;
 
+  // Get the symbol table to update for this object.
+  ValueSymbolTable *ST;
+  if (getSymTab(this, ST))
+    return;  // Cannot set a name on this value (e.g. constant).
+
+  // Avoiding NewName.toStringRef(NameData) initialize and use NameData and
+  // NameRef only if they are needed to set the new name. Even if the new name
+  // is not needed the current name still needs to be removed.
   SmallString<256> NameData;
-  StringRef NameRef = NewName.toStringRef(NameData);
+  StringRef NameRef;
+
+  if ((!getContext().shouldDiscardValueNames() || isa<GlobalValue>(this)) &&
+      !NewName.isTriviallyEmpty()) {
+    NameRef = NewName.toStringRef(NameData);
     assert(NameRef.find_first_of(0) == StringRef::npos &&
            "Null bytes are not allowed in names");
 
     // Name isn't changing?
     if (getName() == NameRef)
       return;
+  }
 
-  assert(!getType()->isVoidTy() && "Cannot assign a name to void values!");
-
-  // Get the symbol table to update for this object.
-  ValueSymbolTable *ST;
-  if (getSymTab(this, ST))
-    return;  // Cannot set a name on this value (e.g. constant).
+  assert((NameRef.empty() || !getType()->isVoidTy()) &&
+         "Cannot assign a name to void values!");
 
   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.
+    if (!NameRef.empty()) {
       MallocAllocator Allocator;
       setValueName(ValueName::create(NameRef, Allocator));
       getValueName()->setValue(this);
+    }
     return;
   }
 
   // NOTE: Could optimize for the case the name is shrinking to not deallocate
   // then reallocated.
   if (hasName()) {
     // Remove old name.
     ST->removeValueName(getValueName());
     destroyValueName();
-
-    if (NameRef.empty())
-      return;
   }
 
   // Name is changing to something new.
+  if (!NameRef.empty())
     setValueName(ST->createValueName(NameRef, this));
 }
 
 void Value::setName(const Twine &NewName) {
   setNameImpl(NewName);
   if (Function *F = dyn_cast<Function>(this))
     F->recalculateIntrinsicID();
 }
 

llvm/unittests/IR/BasicBlockTest.cpp

)");
   BB0->erase(Instr1->getIterator(), Instr2->getIterator());
   EXPECT_EQ(BB0->size(), 2u);
   EXPECT_EQ(&*BB0->begin(), Instr2);
 
   // Erase all instructions
   BB0->erase(BB0->begin(), BB0->end());
   EXPECT_TRUE(BB0->empty());
 }
+
+TEST(BasicBlockTest, DiscardValueNames) {
+  const char *ModuleString = "declare void @f(i32 %dangling)";
+  SMDiagnostic Err;
+  LLVMContext Ctx;
+  { // Scope of M.
+    auto M = parseAssemblyString(ModuleString, Err, Ctx);
+    ASSERT_TRUE(M.get());
+    ASSERT_FALSE(Ctx.shouldDiscardValueNames());
+  }
+  { // Scope of M.
+    auto M = parseAssemblyString(ModuleString, Err, Ctx);
+    ASSERT_TRUE(M.get());
+    Ctx.setDiscardValueNames(true);
+  }
+}
+
+TEST(BasicBlockTest, DiscardValueNames2) {
+  SMDiagnostic Err;
+  LLVMContext Ctx;
+  Module M("Mod", Ctx);
+  auto FTy = FunctionType::get(Type::getVoidTy(M.getContext()),
+                               {Type::getInt32Ty(Ctx)}, /*isVarArg=*/false);
+  { // Scope of F.
+    Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", &M);
+    F->getArg(0)->setName("dangling");
+    F->removeFromParent();
+    ASSERT_FALSE(Ctx.shouldDiscardValueNames());
+    delete F;
+  }
+  { // Scope of F.
+    Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", &M);
+    F->getArg(0)->setName("dangling");
+    F->removeFromParent();
+    Ctx.setDiscardValueNames(true);
+    delete F;
+  }
+}
+
 } // End anonymous namespace.
 } // End llvm namespace.