[SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash #71472

qiongsiwu · 2023-11-07T01:21:40Z

Situations may arise leading to negative NumElements argument of an alloca instruction. In this case the NumElements is treated as a large unsigned value. Such large arrays may cause the size constant to overflow during code generation under 32 bit mode, leading to a crash. This PR limits the constant's bit width to the width of the pointer on the target. With this fix,

alloca i32, i32 -1

and

alloca [4294967295 x i32], i32 1

generates the exact same PowerPC assembly code under 32 bit mode.

llvmbot · 2023-11-07T01:22:14Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-selectiondag

Author: Qiongsi Wu (qiongsiwu)

Changes

instcombine currently generates large arrays when the NumElements argument of an alloca instruction is negative. Such large arrays may cause the size constant to overflow during code generation under 32 bit mode, leading to a crash. This PR limits the constant's bit width to the width of the pointer on the target. With this fix,

alloca i32, i32 -1

and

alloca [4294967295 x i32], i32 1

generates the exact same PowerPC assembly code under 32 bit mode.

Full diff: https://github.com/llvm/llvm-project/pull/71472.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+4-3)
(added) llvm/test/CodeGen/PowerPC/alloca-neg-size.ll (+46)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index aab0d5c5a348bfe..d5ffaf28ca2d499 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4138,9 +4138,10 @@ void SelectionDAGBuilder::visitAlloca(const AllocaInst &I) {
                                           APInt(IntPtr.getScalarSizeInBits(),
                                                 TySize.getKnownMinValue())));
   else
-    AllocSize =
-        DAG.getNode(ISD::MUL, dl, IntPtr, AllocSize,
-                    DAG.getConstant(TySize.getFixedValue(), dl, IntPtr));
+    AllocSize = DAG.getNode(ISD::MUL, dl, IntPtr, AllocSize,
+                            DAG.getConstant(APInt(IntPtr.getScalarSizeInBits(),
+                                                  TySize.getFixedValue()),
+                                            dl, IntPtr));
 
   // Handle alignment.  If the requested alignment is less than or equal to
   // the stack alignment, ignore it.  If the size is greater than or equal to
diff --git a/llvm/test/CodeGen/PowerPC/alloca-neg-size.ll b/llvm/test/CodeGen/PowerPC/alloca-neg-size.ll
new file mode 100644
index 000000000000000..ba22c0a71294b8d
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/alloca-neg-size.ll
@@ -0,0 +1,46 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
+; The instcombine pass can turn
+;     alloca i32, i32 -1
+; to
+;     alloca [4294967295 x i32], i32 1
+; because it zero extends the NumElements to unit64_t.
+; The zero extension can lead to oversized arrays on a 32 bit system.
+; Alloca-ing an array of size bigger than half of the address space
+; is most likely an undefined behaviour, but the code generator
+; should not crash in such situations.
+; RUN: llc < %s -mtriple=powerpc-ibm-aix-xcoff | FileCheck %s
+define void @test_negalloc(ptr %dst, i32 %cond) {
+; CHECK-LABEL: test_negalloc:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    stw 31, -4(1)
+; CHECK-NEXT:    stwu 1, -80(1)
+; CHECK-NEXT:    cmplwi 4, 0
+; CHECK-NEXT:    mr 31, 1
+; CHECK-NEXT:    beq 0, L..BB0_2
+; CHECK-NEXT:  # %bb.1: # %if.then
+; CHECK-NEXT:    li 4, 0
+; CHECK-NEXT:    addi 5, 31, 80
+; CHECK-NEXT:    stwux 5, 1, 4
+; CHECK-NEXT:    addi 4, 1, 32
+; CHECK-NEXT:    b L..BB0_3
+; CHECK-NEXT:  L..BB0_2:
+; CHECK-NEXT:    addi 4, 31, 44
+; CHECK-NEXT:  L..BB0_3: # %if.end
+; CHECK-NEXT:    stw 4, 0(3)
+; CHECK-NEXT:    lwz 1, 0(1)
+; CHECK-NEXT:    lwz 31, -4(1)
+; CHECK-NEXT:    blr
+entry:
+  %0 = alloca [8 x i32], i32 1, align 4
+  %tobool = icmp ne i32 %cond, 0
+  br i1 %tobool, label %if.then, label %if.end
+
+if.then:
+  %vla1 = alloca [4294967295 x i32], i32 1, align 4
+  br label %if.end
+
+if.end:
+  %arr = phi ptr [%0, %entry], [%vla1, %if.then]
+  store ptr %arr, ptr %dst
+  ret void
+}

llvm/test/CodeGen/PowerPC/alloca-neg-size.ll

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

arsenm · 2023-11-07T05:08:08Z

Does GlobalISel need the same change?

qiongsiwu · 2023-11-07T16:21:38Z

Does GlobalISel need the same change?

Thanks for pointing out GlobalISel. I suspect we do not need the exact same change because IRTranslator already takes care of restricting the constant width. See

llvm-project/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp

Line 2861 in 75d6795

getOrCreateVReg(*ConstantInt::get(IntPtrIRTy, DL->getTypeAllocSize(Ty)));

The new test case can proceed to the legalizer with -mtriple=riscv[64|32]-unknown-linux-gnu -global-isel, but both fail in the legalizer with errors

# 64 bit
LLVM ERROR: unable to legalize instruction: %14:_(p0) = G_DYN_STACKALLOC %13:_(s64), 1 (in function: test_oversized)
# 32 bit
LLVM ERROR: unable to legalize instruction: %12:_(p0) = G_DYN_STACKALLOC %11:_(s32), 1 (in function: test_oversized)

This does not look like the same issue as the one this PR is fixing. If it is reasonable, I prefer leaving this PR fixing the selection DAG, and open an separate issue for GlobalISel.

Does this sound reasonable?

qiongsiwu · 2023-11-08T14:27:40Z

@arsenm Hi Matt! The PR description is also updated to avoid mentioning any specific optimization passes. Earlier change requests are either addressed, or I have some follow up questions and I would like to seek your clarification (here and here).

Could you get back to me on the two questions? Thanks so much!

qiongsiwu · 2023-11-09T15:55:01Z

Hi @arsenm! Could you provide some feedback/clarifications? We would like to get this fix going since there are other work that depends on it. I appreciate your timely feedback!

Thanks!

arsenm · 2023-11-10T08:42:04Z

This does not look like the same issue as the one this PR is fixing. If it is reasonable, I prefer leaving this PR fixing the selection DAG, and open an separate issue for GlobalISel.

Does this sound reasonable?

Sounds like there's no issue, and could just use a test. The target also doesn't matter, you should be able to use aarch64 with fewer issues

qiongsiwu · 2023-11-10T20:41:14Z

This does not look like the same issue as the one this PR is fixing. If it is reasonable, I prefer leaving this PR fixing the selection DAG, and open an separate issue for GlobalISel.
Does this sound reasonable?

Sounds like there's no issue, and could just use a test. The target also doesn't matter, you should be able to use aarch64 with fewer issues

Thanks! The test is revised to cover aarch64 GlobalISel in addition to powerpc.

arsenm

lgtm with nit

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

qiongsiwu · 2023-11-14T15:52:36Z

Landing this PR now since it technically is approved (#71472 (review)) and it passes all checks. Thanks again for your inputs @arsenm !

…void Code Generator Crash (llvm#71472) Situations may arise leading to negative `NumElements` argument of an `alloca` instruction. In this case the `NumElements` is treated as a large unsigned value. Such large arrays may cause the size constant to overflow during code generation under 32 bit mode, leading to a crash. This PR limits the constant's bit width to the width of the pointer on the target. With this fix, ``` alloca i32, i32 -1 ``` and ``` alloca [4294967295 x i32], i32 1 ``` generates the exact same PowerPC assembly code under 32 bit mode.

chfast · 2023-11-21T18:06:25Z

This is related to #63377.

Fix code generator crash when it sees an oversized alloca type

2e98ef4

qiongsiwu self-assigned this Nov 7, 2023

llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Nov 7, 2023

qiongsiwu requested a review from kmclaughlin-arm November 7, 2023 01:22

qiongsiwu requested review from RolandF77, scui-ibm, w2yehia and arsenm November 7, 2023 01:22

qiongsiwu mentioned this pull request Nov 7, 2023

[InstCombine] Avoid Allocating Arrays Too Large For the Target #70980

Closed

arsenm requested changes Nov 7, 2023

View reviewed changes

llvm/test/CodeGen/PowerPC/alloca-neg-size.ll Outdated Show resolved Hide resolved

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Outdated Show resolved Hide resolved

Address review comments in the test

e978491

qiongsiwu changed the title ~~[CodeGen] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash~~ [SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash Nov 7, 2023

qiongsiwu requested a review from arsenm November 8, 2023 14:28

Address review comments

3fe11dc

llvmbot added the backend:AArch64 label Nov 13, 2023

Revise tests

35388a2

arsenm reviewed Nov 14, 2023

View reviewed changes

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Outdated Show resolved Hide resolved

Address review comment

4757779

qiongsiwu merged commit c8b1109 into llvm:main Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash #71472

[SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash #71472

Uh oh!

qiongsiwu commented Nov 7, 2023 •

edited

Loading

Uh oh!

llvmbot commented Nov 7, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

arsenm commented Nov 7, 2023

Uh oh!

qiongsiwu commented Nov 7, 2023 •

edited

Loading

Uh oh!

qiongsiwu commented Nov 8, 2023

Uh oh!

qiongsiwu commented Nov 9, 2023

Uh oh!

arsenm commented Nov 10, 2023

Uh oh!

qiongsiwu commented Nov 10, 2023

Uh oh!

arsenm left a comment

Uh oh!

Uh oh!

qiongsiwu commented Nov 14, 2023

Uh oh!

chfast commented Nov 21, 2023

Uh oh!

Uh oh!

[SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash #71472

[SelectionDAG] Handling Oversized Alloca Types under 32 bit Mode to Avoid Code Generator Crash #71472

Uh oh!

Conversation

qiongsiwu commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arsenm commented Nov 7, 2023

Uh oh!

qiongsiwu commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiongsiwu commented Nov 8, 2023

Uh oh!

qiongsiwu commented Nov 9, 2023

Uh oh!

arsenm commented Nov 10, 2023

Uh oh!

qiongsiwu commented Nov 10, 2023

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qiongsiwu commented Nov 14, 2023

Uh oh!

chfast commented Nov 21, 2023

Uh oh!

Uh oh!

qiongsiwu commented Nov 7, 2023 •

edited

Loading

llvmbot commented Nov 7, 2023 •

edited

Loading

qiongsiwu commented Nov 7, 2023 •

edited

Loading