[llvm] [AMDGPU] Fix leak and self-assignment in copy assignment operator (PR #107847)

[Fraser Cormack via llvm-commits]

https://github.com/frasercrmck created https://github.com/llvm/llvm-project/pull/107847

A static analyzer identified that this operator was unsafe in the case of self-assignment.

In the placement new statement, StringValue's copy constructor was being implicitly called, which received a reference to "itself". In fact, it was being passed an old StringValue at the same address - one whose lifetime had already ended. The copy constructor was thus copying fields from a dead object.

We need to be careful when switching active union members, and calling the destructor on the old StringValue will avoid memory leaks which I believe the old code exhibited.

>From c1f08478277793598ac6702509e65697ef784b4e Mon Sep 17 00:00:00 2001
From: Fraser Cormack <fraser at codeplay.com>
Date: Mon, 9 Sep 2024 12:38:17 +0100
Subject: [PATCH] [AMDGPU] Fix leak and self-assignment in copy assignment
 operator

A static analyzer identified that this operator was unsafe in the case
of self-assignment.

In the placement new statement, StringValue's copy constructor was being
implicitly called, which received a reference to "itself". In fact, it
was being passed an old StringValue at the same address - one whose
lifetime had already ended. The copy constructor was thus copying fields
from a dead object.

We need to be careful when switching active union members, and calling
the destructor on the old StringValue will avoid memory leaks which I
believe the old code exhibited.
---
 .../lib/Target/AMDGPU/SIMachineFunctionInfo.h | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h b/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
index 7af5e7388f841e..4cc60f50978996 100644
--- a/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
@@ -100,19 +100,25 @@ struct SIArgument {
   SIArgument() : IsRegister(false), StackOffset(0) {}
   SIArgument(const SIArgument &Other) {
     IsRegister = Other.IsRegister;
-    if (IsRegister) {
-      ::new ((void *)std::addressof(RegisterName))
-          StringValue(Other.RegisterName);
-    } else
+    if (IsRegister)
+      new (&RegisterName) StringValue(Other.RegisterName);
+    else
       StackOffset = Other.StackOffset;
     Mask = Other.Mask;
   }
   SIArgument &operator=(const SIArgument &Other) {
+    // Default-construct or destruct the old RegisterName in case of switching
+    // union members
+    if (IsRegister != Other.IsRegister) {
+      if (Other.IsRegister)
+        new (&RegisterName) StringValue();
+      else
+        RegisterName.~StringValue();
+    }
     IsRegister = Other.IsRegister;
-    if (IsRegister) {
-      ::new ((void *)std::addressof(RegisterName))
-          StringValue(Other.RegisterName);
-    } else
+    if (IsRegister)
+      RegisterName = Other.RegisterName;
+    else
       StackOffset = Other.StackOffset;
     Mask = Other.Mask;
     return *this;