[llvm] [PPCMergeStringPool] Only replace constant once (PR #92996)

[Nikita Popov via llvm-commits]

https://github.com/nikic updated https://github.com/llvm/llvm-project/pull/92996

>From 870147d977f7bedccc4986992e5652227ddf4ffb Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 22 May 2024 08:39:50 +0200
Subject: [PATCH 1/3] [PPCMergeStringPool] Only replace constant once

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. I wasn't able to come up with a test case though.

This is intended to fix https://github.com/llvm/llvm-project/issues/92991.
---
 .../lib/Target/PowerPC/PPCMergeStringPool.cpp | 37 ++++---------------
 1 file changed, 7 insertions(+), 30 deletions(-)

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

>From d53dbb9e0e7658d550f3014b1009c083cad9c5bd Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Wed, 22 May 2024 08:48:05 +0200
Subject: [PATCH 2/3] clang-format

---
 llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
index b73e25d37f8f5..309938accdf4c 100644
--- a/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
+++ b/llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
@@ -312,8 +312,8 @@ void PPCMergeStringPool::replaceUsesWithGEP(GlobalVariable *GlobalToReplace,
   Indices.push_back(ConstantInt::get(Type::getInt32Ty(*Context), 0));
   Indices.push_back(ConstantInt::get(Type::getInt32Ty(*Context), ElementIndex));
 
-  Constant *ConstGEP = ConstantExpr::getInBoundsGetElementPtr(
-      PooledStructType, GPool, Indices);
+  Constant *ConstGEP =
+      ConstantExpr::getInBoundsGetElementPtr(PooledStructType, GPool, Indices);
   LLVM_DEBUG(dbgs() << "Replacing this global:\n");
   LLVM_DEBUG(GlobalToReplace->dump());
   LLVM_DEBUG(dbgs() << "with this:\n");

>From c6ab5a3a252961713349ba8992cfb8385b21c8ca Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 23 May 2024 09:04:56 +0200
Subject: [PATCH 3/3] Add test case

This previously produced use-after-free under asan.
---
 .../PowerPC/mergeable-string-pool-pr92991.ll  | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 llvm/test/CodeGen/PowerPC/mergeable-string-pool-pr92991.ll

diff --git a/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pr92991.ll b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pr92991.ll
new file mode 100644
index 0000000000000..4e9c69e5fe4cf
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/mergeable-string-pool-pr92991.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+
+ at g = private constant [4 x i32] [i32 122, i32 67, i32 35, i32 56]
+ at g2 = private constant [1 x i64] [i64 1], align 8
+
+define void @test(ptr %p, ptr %p2) {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    addis 5, 2, .L__ModuleStringPool at toc@ha
+; CHECK-NEXT:    addi 5, 5, .L__ModuleStringPool at toc@l
+; CHECK-NEXT:    addi 6, 5, 12
+; CHECK-NEXT:    std 6, 0(3)
+; CHECK-NEXT:    addi 3, 5, 16
+; CHECK-NEXT:    std 3, 0(4)
+; CHECK-NEXT:    blr
+  store ptr getelementptr inbounds ([4 x i32], ptr @g, i64 0, i64 1), ptr %p
+  store ptr getelementptr inbounds ([4 x i32], ptr @g, i64 0, i64 2), ptr %p2
+  ret void
+}