efriedma added inline comments.
================ Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3460 + } else if (EffectiveTriple.isArm() || EffectiveTriple.isThumb()) { + CmdArgs.push_back("-mstack-probe-size=1024"); + } ---------------- Why 1024? ================ Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:780 emitSPUpdate(isARM, MBB, MBBI, dl, TII, -NumBytes, MachineInstr::FrameSetup); DefCFAOffsetCandidates.addInst(std::prev(MBBI), NumBytes, true); ---------------- Is this relevant? Are the other unmodified uses of emitSPUpdate() relevant? ================ Comment at: llvm/lib/Target/ARM/ARMFrameLowering.cpp:979 if (STI.isTargetWindows() && WindowsRequiresStackProbe(MF, NumBytes)) { uint32_t NumWords = NumBytes >> 2; ---------------- Can we try to unify the Windows/non-Windows codepaths to some extent? Having two independent codepaths each trying to decide when probing is necessary seems likely to lead to bugs. ================ Comment at: llvm/test/CodeGen/ARM/stackProbing_arm.ll:19 +; CHECK-NEXT: .pad #4096 +; CHECK-NEXT: sub sp, sp, #4096 +; CHECK-NEXT: cmp sp, r0 ---------------- Going straight from the function entry to here, you've decremented the stack by a total of 6276 bytes; this can jump over a 4kb guard page. ================ Comment at: llvm/test/CodeGen/ARM/stackProbing_arm.ll:21 +; CHECK-NEXT: cmp sp, r0 +; CHECK-NEXT: str r0, [sp, #1024] +; CHECK-NEXT: bne .LBB0_1 ---------------- From a security perspective, scattering pointers to the stack onto the stack is maybe not the best idea. ================ Comment at: llvm/test/CodeGen/ARM/stackProbing_arm.ll:95 +; CHECK-NEXT: @ =>This Inner Loop Header: Depth=1 +; CHECK-NEXT: sub sp, sp, #4096 +; CHECK-NEXT: cmp sp, r3 ---------------- Again, this is too many bytes. (At least, ignoring the stores before the loop, which don't appear to be intentionally inserted checks.) ================ Comment at: llvm/test/CodeGen/ARM/stackProbing_thumb.ll:12 +; CHECK-NEXT: ldr r0, .LCPI0_1 +; CHECK-NEXT: subs r0, r0, r0 +; CHECK-NEXT: ldr r1, .LCPI0_2 ---------------- This is a very fancy way of setting a register to zero. Have you done any testing that the generated code actually works? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154911/new/ https://reviews.llvm.org/D154911 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits