[PATCH] D143487: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames
Yevgeny Rouban via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 7 04:03:41 PST 2023
yrouban created this revision.
yrouban added reviewers: efriedma, jsilvanus, tejohnson, arsenm.
Herald added a project: All.
yrouban requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.
//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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D143487
Files:
llvm/lib/IR/Value.cpp
llvm/unittests/IR/BasicBlockTest.cpp
Index: llvm/unittests/IR/BasicBlockTest.cpp
===================================================================
--- llvm/unittests/IR/BasicBlockTest.cpp
+++ llvm/unittests/IR/BasicBlockTest.cpp
@@ -541,5 +541,44 @@
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);
+ EXPECT_TRUE(M.get());
+ EXPECT_FALSE(Ctx.shouldDiscardValueNames());
+ }
+ { // Scope of M.
+ auto M = parseAssemblyString(ModuleString, Err, Ctx);
+ EXPECT_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.
Index: llvm/lib/IR/Value.cpp
===================================================================
--- llvm/lib/IR/Value.cpp
+++ llvm/lib/IR/Value.cpp
@@ -315,10 +315,6 @@
}
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;
@@ -339,6 +335,9 @@
if (getSymTab(this, ST))
return; // Cannot set a name on this value (e.g. constant).
+ bool NeedNewName =
+ !getContext().shouldDiscardValueNames() || isa<GlobalValue>(this);
+
if (!ST) { // No symbol table to update? Just do the change.
if (NameRef.empty()) {
// Free the name for this value.
@@ -351,9 +350,11 @@
destroyValueName();
// Create the new name.
- MallocAllocator Allocator;
- setValueName(ValueName::create(NameRef, Allocator));
- getValueName()->setValue(this);
+ if (NeedNewName) {
+ MallocAllocator Allocator;
+ setValueName(ValueName::create(NameRef, Allocator));
+ getValueName()->setValue(this);
+ }
return;
}
@@ -369,7 +370,8 @@
}
// Name is changing to something new.
- setValueName(ST->createValueName(NameRef, this));
+ if (NeedNewName)
+ setValueName(ST->createValueName(NameRef, this));
}
void Value::setName(const Twine &NewName) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D143487.495457.patch
Type: text/x-patch
Size: 3038 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230207/aae1138d/attachment.bin>
More information about the llvm-commits
mailing list