[llvm] [AMDGPU] Create local KnownBits in case DenseMap gets invalidated (PR #111568)
Janek van Oirschot via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 07:27:18 PDT 2024
https://github.com/JanekvO updated https://github.com/llvm/llvm-project/pull/111568
>From e241240e9d0d58d15fded9b1e9d6b5c3f0318569 Mon Sep 17 00:00:00 2001
From: Janek van Oirschot <janek.vanoirschot at amd.com>
Date: Tue, 8 Oct 2024 18:34:32 +0100
Subject: [PATCH 1/2] [AMDGPU] Create local KnownBits in case DenseMap gets
invalidated
---
.../AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp | 8 +-
.../AMDGPU/mcexpr-knownbits-assign-crash.ll | 120 ++++++++++++++++++
2 files changed, 126 insertions(+), 2 deletions(-)
create mode 100644 llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll
diff --git a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp
index 4fbd7d0f889457..9f5f4399679376 100644
--- a/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp
+++ b/llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCExpr.cpp
@@ -537,8 +537,12 @@ static void knownBitsMapHelper(const MCExpr *Expr, KnownBitsMap &KBM,
// Variable value retrieval is not for actual use but only for knownbits
// analysis.
- knownBitsMapHelper(Sym.getVariableValue(/*SetUsed=*/false), KBM, Depth + 1);
- KBM[Expr] = KBM[Sym.getVariableValue(/*SetUsed=*/false)];
+ const MCExpr *SymVal = Sym.getVariableValue(/*setUsed=*/false);
+ knownBitsMapHelper(SymVal, KBM, Depth + 1);
+
+ // Explicitly copy-construct so that there exists a local KnownBits in case
+ // KBM[SymVal] gets invalidated after a potential growth through KBM[Expr].
+ KBM[Expr] = KnownBits(KBM[SymVal]);
return;
}
case MCExpr::ExprKind::Unary: {
diff --git a/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll b/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll
new file mode 100644
index 00000000000000..830996d2e24023
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll
@@ -0,0 +1,120 @@
+; REQUIRES: asserts
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1030 -o - %s 2>&1 | FileCheck %s
+
+; CHECK-NOT: Assertion `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"' failed
+
+define void @RunTic() {
+ %call5.i1 = call i32 @G_CheckDemoStatus()
+ tail call void @D_AdvanceDemo()
+ call void @G_Ticker()
+ ret void
+}
+
+define void @G_Ticker() {
+ call void @G_DoReborn()
+ tail call void @F_Ticker()
+ tail call void @AM_Stop()
+ tail call void @F_StartFinale()
+ tail call void @D_AdvanceDemo()
+ %call.i.i449 = call i32 @R_FlatNumForName()
+ %call9.i.i = call i32 @R_TextureNumForName()
+ %call.i306 = tail call ptr @P_TempSaveGameFile()
+ %call1.i307 = call ptr @P_SaveGameFile()
+ call void (...) @I_Error()
+ ret void
+}
+
+define void @G_DoReborn() {
+ call void @P_RemoveMobj()
+ call void @P_SpawnMobj()
+ call void @P_SpawnPlayer()
+ call void (...) @I_Error()
+ ret void
+}
+
+define void @AM_Stop() {
+ ret void
+}
+
+define void @D_AdvanceDemo() {
+ ret void
+}
+
+define void @F_StartFinale() {
+ ret void
+}
+
+define void @F_Ticker() {
+ ret void
+}
+
+define void @G_PlayerReborn() {
+ ret void
+}
+
+define i32 @G_CheckDemoStatus() {
+ tail call void @I_Quit()
+ tail call void @D_AdvanceDemo()
+ call void (...) @I_Error()
+ ret i32 0
+}
+
+define void @HU_Start() {
+ ret void
+}
+
+define void @I_Quit() {
+ %fptr = load ptr, ptr null, align 8
+ tail call void %fptr()
+ ret void
+}
+
+define void @P_SetThingPosition() {
+ ret void
+}
+
+define void @P_RemoveMobj() {
+ ret void
+}
+
+define void @P_SpawnMobj() {
+ ret void
+}
+
+define void @P_SpawnPlayer() {
+ call void @G_PlayerReborn()
+ call void @P_SetThingPosition()
+ call void @P_SetupPsprites(ptr addrspace(1) null)
+ tail call void @HU_Start()
+ ret void
+}
+
+define void @P_SetupPsprites(ptr addrspace(1) %i) {
+ %fptr = load ptr, ptr addrspace(1) %i, align 8
+ tail call void %fptr()
+ ret void
+}
+
+define ptr @P_TempSaveGameFile() {
+ ret ptr null
+}
+
+define ptr @P_SaveGameFile() {
+ ret ptr null
+}
+
+define i32 @R_TextureNumForName() {
+ %ret = call i32 @R_FlatNumForName()
+ ret i32 0
+}
+
+define void @I_Error(...) {
+ %fptr = load ptr, ptr null, align 8
+ call void %fptr()
+ ret void
+}
+
+define i32 @R_FlatNumForName() {
+ call void (...) @I_Error()
+ unreachable
+}
>From ac7eec6e9284ae92392c101d798582a198c0e212 Mon Sep 17 00:00:00 2001
From: Janek van Oirschot <janek.vanoirschot at amd.com>
Date: Wed, 9 Oct 2024 15:26:51 +0100
Subject: [PATCH 2/2] Rework and rename test case
---
...knownbits-assign-crash-gh-issue-110930.ll} | 138 ++++++++++--------
1 file changed, 81 insertions(+), 57 deletions(-)
rename llvm/test/CodeGen/AMDGPU/{mcexpr-knownbits-assign-crash.ll => mcexpr-knownbits-assign-crash-gh-issue-110930.ll} (67%)
diff --git a/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll b/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash-gh-issue-110930.ll
similarity index 67%
rename from llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll
rename to llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash-gh-issue-110930.ll
index 830996d2e24023..deb30aeb297b61 100644
--- a/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash.ll
+++ b/llvm/test/CodeGen/AMDGPU/mcexpr-knownbits-assign-crash-gh-issue-110930.ll
@@ -1,120 +1,144 @@
-; REQUIRES: asserts
-; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1030 -o - %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx1030 < %s | FileCheck %s
-; CHECK-NOT: Assertion `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"' failed
+; Previously, this would hit an assertion on incompatible comparison between
+; APInts due to BitWidth differences. This was due to assignment of DenseMap
+; value using another value within that same DenseMap which results in a
+; use-after-free if the assignment operator invokes a DenseMap growth.
-define void @RunTic() {
- %call5.i1 = call i32 @G_CheckDemoStatus()
- tail call void @D_AdvanceDemo()
- call void @G_Ticker()
+; CHECK-LABEL: I_Quit:
+define void @I_Quit() {
+ %fptr = load ptr, ptr null, align 8
+ tail call void %fptr()
ret void
}
-define void @G_Ticker() {
- call void @G_DoReborn()
- tail call void @F_Ticker()
- tail call void @AM_Stop()
- tail call void @F_StartFinale()
- tail call void @D_AdvanceDemo()
- %call.i.i449 = call i32 @R_FlatNumForName()
- %call9.i.i = call i32 @R_TextureNumForName()
- %call.i306 = tail call ptr @P_TempSaveGameFile()
- %call1.i307 = call ptr @P_SaveGameFile()
- call void (...) @I_Error()
+; CHECK-LABEL: P_RemoveMobj:
+define void @P_RemoveMobj() {
ret void
}
-define void @G_DoReborn() {
- call void @P_RemoveMobj()
- call void @P_SpawnMobj()
- call void @P_SpawnPlayer()
- call void (...) @I_Error()
+; CHECK-LABEL: P_SpawnMobj:
+define void @P_SpawnMobj() {
ret void
}
-define void @AM_Stop() {
+; CHECK-LABEL: G_PlayerReborn:
+define void @G_PlayerReborn() {
ret void
}
-define void @D_AdvanceDemo() {
+; CHECK-LABEL: P_SetThingPosition:
+define void @P_SetThingPosition() {
ret void
}
-define void @F_StartFinale() {
+; CHECK-LABEL: P_SetupPsprites:
+define void @P_SetupPsprites(ptr addrspace(1) %i) {
+ %fptr = load ptr, ptr addrspace(1) %i, align 8
+ tail call void %fptr()
ret void
}
-define void @F_Ticker() {
+; CHECK-LABEL: HU_Start:
+define void @HU_Start() {
ret void
}
-define void @G_PlayerReborn() {
+; CHECK-LABEL: P_SpawnPlayer:
+define void @P_SpawnPlayer() {
+ call void @G_PlayerReborn()
+ call void @P_SetThingPosition()
+ call void @P_SetupPsprites(ptr addrspace(1) null)
+ tail call void @HU_Start()
ret void
}
-define i32 @G_CheckDemoStatus() {
- tail call void @I_Quit()
- tail call void @D_AdvanceDemo()
- call void (...) @I_Error()
- ret i32 0
-}
-
-define void @HU_Start() {
+; CHECK-LABEL: I_Error:
+define void @I_Error(...) {
+ %fptr = load ptr, ptr null, align 8
+ call void %fptr()
ret void
}
-define void @I_Quit() {
- %fptr = load ptr, ptr null, align 8
- tail call void %fptr()
+; CHECK-LABEL: G_DoReborn:
+define void @G_DoReborn() {
+ call void @P_RemoveMobj()
+ call void @P_SpawnMobj()
+ call void @P_SpawnPlayer()
+ call void (...) @I_Error()
ret void
}
-define void @P_SetThingPosition() {
+; CHECK-LABEL: AM_Stop:
+define void @AM_Stop() {
ret void
}
-define void @P_RemoveMobj() {
+; CHECK-LABEL: D_AdvanceDemo:
+define void @D_AdvanceDemo() {
ret void
}
-define void @P_SpawnMobj() {
+; CHECK-LABEL: F_StartFinale:
+define void @F_StartFinale() {
ret void
}
-define void @P_SpawnPlayer() {
- call void @G_PlayerReborn()
- call void @P_SetThingPosition()
- call void @P_SetupPsprites(ptr addrspace(1) null)
- tail call void @HU_Start()
+; CHECK-LABEL: F_Ticker:
+define void @F_Ticker() {
ret void
}
-define void @P_SetupPsprites(ptr addrspace(1) %i) {
- %fptr = load ptr, ptr addrspace(1) %i, align 8
- tail call void %fptr()
- ret void
+; CHECK-LABEL: G_CheckDemoStatus:
+define i32 @G_CheckDemoStatus() {
+ tail call void @I_Quit()
+ tail call void @D_AdvanceDemo()
+ call void (...) @I_Error()
+ ret i32 0
}
+
+; CHECK-LABEL: P_TempSaveGameFile:
define ptr @P_TempSaveGameFile() {
ret ptr null
}
+; CHECK-LABEL: P_SaveGameFile:
define ptr @P_SaveGameFile() {
ret ptr null
}
+; CHECK-LABEL: R_FlatNumForName:
+define i32 @R_FlatNumForName() {
+ call void (...) @I_Error()
+ unreachable
+}
+
+; CHECK-LABEL: R_TextureNumForName:
define i32 @R_TextureNumForName() {
%ret = call i32 @R_FlatNumForName()
ret i32 0
}
-define void @I_Error(...) {
- %fptr = load ptr, ptr null, align 8
- call void %fptr()
+; CHECK-LABEL: G_Ticker:
+define void @G_Ticker() {
+ call void @G_DoReborn()
+ tail call void @F_Ticker()
+ tail call void @AM_Stop()
+ tail call void @F_StartFinale()
+ tail call void @D_AdvanceDemo()
+ %call.i.i449 = call i32 @R_FlatNumForName()
+ %call9.i.i = call i32 @R_TextureNumForName()
+ %call.i306 = tail call ptr @P_TempSaveGameFile()
+ %call1.i307 = call ptr @P_SaveGameFile()
+ call void (...) @I_Error()
ret void
}
-define i32 @R_FlatNumForName() {
- call void (...) @I_Error()
- unreachable
+; CHECK-LABEL: RunTic:
+define void @RunTic() {
+ %call5.i1 = call i32 @G_CheckDemoStatus()
+ tail call void @D_AdvanceDemo()
+ call void @G_Ticker()
+ ret void
}
More information about the llvm-commits
mailing list