[PATCH] D40863: [AArch64][Darwin] Implement stack probing for static and dynamic stack objects

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In https://reviews.llvm.org/D40863#953298, @mstorsjo wrote:

> In https://reviews.llvm.org/D40863#945847, @aemerson wrote:
>
> > In https://reviews.llvm.org/D40863#945833, @MatzeB wrote:
> >
> > > It would be helpful to describe somewhere what exactly stack probing is. Maybe add more comments to emitStackProbe() and the commit message.
> > >
> > > Is it one of those situations where you have to touch the OS pages backing stack memory one after the other (instead of accidentally jumping across pages for big arrays)?
> >
> >
> > Yes, that's correct. The default implementation I've created in https://reviews.llvm.org/D40857 in compiler-rt loads data in 4096 (=page size) byte intervals. This ensures that the stack guard page is always hit
>
>
> Since aarch64 on darwin isn't exactly new, how did expanding the stack by more than 4096 bytes work prior to this patchset? Functions allocating more than 4096 bytes of stack space isn't exactly uncommon. Will darwin start requiring this if it detects an executable built by a new enough toolchain to support it?


The motivation for stack probing here is different to the Windows case, although the mechanism is the same. For Darwin this is a security feature to protect against stack clash based attacks.



================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:478
+  if (!LRIsSaved) {
+    // LR wasn't saved as a CSR, so we need to save it ourselves. We don't
+    // bother updating SP, as we know the probe function won't modify any
----------------
mstorsjo wrote:
> Instead of manually saving LR here if it wasn't already saved, in D41131 I instead tried to estimate whether the stack probe would end up being necessary in determineCalleeSaves, and made sure LR is saved already there if a stack probe will be necessary.
Is that estimate always conservative? I.e. does it always at least over-estimate the stack size?


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:502
+
+  BuildMI(MBB, MBBI, DL, TII->get(AArch64::BL))
+      .addExternalSymbol(MF.createExternalSymbolName(Symbol))
----------------
mstorsjo wrote:
> In the implementation in D41131 (based on the one for Windows on ARM by @compnerd in rL207615), I check for CodeModel::Large as well, and do the function call via MOVaddrEXT and BLR for that case.
This function is specific to the Darwin stack probing ABI under consideration, so I don't think we'll be able to share this code. No objections to your patch though.


Repository:
  rL LLVM

https://reviews.llvm.org/D40863