[llvm] c53d807 - [IR] Allow destruction of symbol table entries regardless of DiscardValueNames
Yevgeny Rouban via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 30 21:45:06 PDT 2023
Author: Yevgeny Rouban
Date: 2023-03-31T11:44:05+07:00
New Revision: c53d807321f680f81c0599e4395be480dec1ee3e
URL: https://github.com/llvm/llvm-project/commit/c53d807321f680f81c0599e4395be480dec1ee3e
DIFF: https://github.com/llvm/llvm-project/commit/c53d807321f680f81c0599e4395be480dec1ee3e.diff
LOG: [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
Added:
Modified:
llvm/lib/IR/Value.cpp
llvm/unittests/IR/BasicBlockTest.cpp
Removed:
################################################################################
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index 281594d735e0f..11dc5060edfd0 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));
}
diff --git a/llvm/unittests/IR/BasicBlockTest.cpp b/llvm/unittests/IR/BasicBlockTest.cpp
index ead08eabb40d1..454cafe3fe721 100644
--- a/llvm/unittests/IR/BasicBlockTest.cpp
+++ b/llvm/unittests/IR/BasicBlockTest.cpp
@@ -539,5 +539,44 @@ TEST(BasicBlockTest, EraseRange) {
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());
+ EXPECT_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();
+ EXPECT_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.
More information about the llvm-commits
mailing list