[PPCMergeStringPool] Only replace constant once#92996
Conversation
|
@llvm/pr-subscribers-backend-powerpc Author: Nikita Popov (nikic) ChangesIn #88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. I think the case where this happens is constant users where the replacement might result in the destruction of the original constant. I wasn't able to come up with a test case though. This is intended to fix #92991. Full diff: https://github.com/llvm/llvm-project/pull/92996.diff 1 Files Affected:
diff --git a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
index abc5353e4a5e9..b73e25d37f8f5 100644
--- a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
+++ b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
@@ -302,13 +302,6 @@ bool PPCMergeStringPool::mergeModuleStringPool(Module &M) {
return true;
}
-static bool userHasOperand(User *TheUser, GlobalVariable *GVOperand) {
- for (Value *Op : TheUser->operands())
- if (Op == GVOperand)
- return true;
- return false;
-}
-
// For pooled strings we need to add the offset into the pool for each string.
// This is done by adding a Get Element Pointer (GEP) before each user. This
// function adds the GEP.
@@ -319,29 +312,13 @@ void PPCMergeStringPool::replaceUsesWithGEP(GlobalVariable *GlobalToReplace,
Indices.push_back(ConstantInt::get(Type::getInt32Ty(*Context), 0));
Indices.push_back(ConstantInt::get(Type::getInt32Ty(*Context), ElementIndex));
- // Need to save a temporary copy of each user list because we remove uses
- // as we replace them.
- SmallVector<User *> Users;
- for (User *CurrentUser : GlobalToReplace->users())
- Users.push_back(CurrentUser);
-
- for (User *CurrentUser : Users) {
- // The user was not found so it must have been replaced earlier.
- if (!userHasOperand(CurrentUser, GlobalToReplace))
- continue;
-
- // We cannot replace operands in globals so we ignore those.
- if (isa<GlobalValue>(CurrentUser))
- continue;
-
- Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
- PooledStructType, GPool, Indices);
- LLVM_DEBUG(dbgs() << "Replacing this global:\n");
- LLVM_DEBUG(GlobalToReplace->dump());
- LLVM_DEBUG(dbgs() << "with this:\n");
- LLVM_DEBUG(ConstGEP->dump());
- GlobalToReplace->replaceAllUsesWith(ConstGEP);
- }
+ Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
+ PooledStructType, GPool, Indices);
+ LLVM_DEBUG(dbgs() << "Replacing this global:\n");
+ LLVM_DEBUG(GlobalToReplace->dump());
+ LLVM_DEBUG(dbgs() << "with this:\n");
+ LLVM_DEBUG(ConstGEP->dump());
+ GlobalToReplace->replaceAllUsesWith(ConstGEP);
}
} // namespace
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. I think the case where this happens is constant users where the replacement might result in the destruction of the original constant. I wasn't able to come up with a test case though. This is intended to fix llvm#92991.
This previously produced use-after-free under asan.
|
I've added a test case now. It would previously fail when running with asan. |
|
Thank you for adding a test case! It is possible that I have done something wrong here but all I did was rewind to before your change and then copy the test in. |
|
@stefanp-ibm Does your LLVM build enable |
stefanp-synopsys
left a comment
There was a problem hiding this comment.
@stefanp-ibm Does your LLVM build enable
-DLLVM_USE_SANITIZER=Address? Alternatively, invoking opt under valgrind should also show the issue.
I was able to add -DLLVM_USE_SANITIZER=Address to the build and get this test to fail as expected. Thank you for fixing this!
LGTM.
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes llvm#92991. (cherry picked from commit 9f85bc8)
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes llvm#92991. (cherry picked from commit 9f85bc8)
In llvm#88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free. The case where this happens is constant users where the replacement might result in the destruction of the original constant. Fixes llvm#92991. (cherry picked from commit 9f85bc8)
In #88846 I changed this code to use RAUW to perform the replacement instead of manual updates -- but kept the outer loop, which means we try to perform RAUW once per user. However, some of the users might be freed by the RAUW operation, resulting in use-after-free.
I think the case where this happens is constant users where the replacement might result in the destruction of the original constant.
Fixes #92991.