[AMDGPU] Fix leak and self-assignment in copy assignment operator #107847
Conversation
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.
|
@llvm/pr-subscribers-backend-amdgpu Author: Fraser Cormack (frasercrmck) ChangesA 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. Full diff: https://github.com/llvm/llvm-project/pull/107847.diff 1 Files Affected:
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;
|
Thanks! It's an Intel internal static analysis tool. |
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.