[PATCH] D154911: Enabling fstack_clash_protection for arm32 bit, thumb and thumb2 mode

[Eli Friedman via Phabricator via cfe-commits]

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