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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: lib/Target/AArch64/AArch64FrameLowering.cpp:471-487
+  // If the LR has already been saved we don't need to save it before calling
+  // the probe function. However if it hasn't then the probe will clobber it.
+  bool LRIsSaved =
+      std::any_of(CSI.begin(), CSI.end(), [](const CalleeSavedInfo &SI) {
+        return SI.getReg() == AArch64::LR;
+      });
+  if (!LRIsSaved) {
----------------
aemerson wrote:
> MatzeB wrote:
> > Generally it feels sketchy to save/extend/restore the stackframe without MachineFrameInfo knowing about it. (For example you will hide this extra stuff from the `WarnStackSize` functionality in PrologueEpilogueINserter. That's probably not a big deal but if we can somehow find a better way that would be nice.
> > 
> > "We don't bother updating SP...", this is problematic, AFAIK a unix signal can come in at any time and will use your stack frame. It will probably work on platforms with stack red zones defined that the signal handlers have to respect.
> I think I should update MFI to ensure `hasCalls = true` here and in the dynamic case. However for the stack size, since the SP is only ever adjusted by 16 bytes, and then restored after probing, I don't think it's necessary to tell MFI about it since it's purely a local change. Since we must have at least 4k of stack objects in order to be attempting to probe, the 16 bytes are only using stack space that would have been re-used later anyway.
> 
> > "We don't bother updating SP...", this is problematic, AFAIK a unix signal can come in at any time and will use your stack frame. It will probably work on platforms with stack red zones defined that the signal handlers  have to respect.
> 
> I think that's a mis-worded comment. I meant to say that we don't bother saving SP since we know the probe function won't clobber it. However that might not be a good idea to restrict it so much in future. The ABI isn't yet 100% finalized.
Ah, I didn't realize this happens before the stackframe is even setup so it doesn't effect the maximum or the layout during the main part of the function.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:11040-11042
+  if (Subtarget->isTargetDarwin() && MF.getTarget().Options.EnableStackProbe)
+    return "___chkstk_darwin";
+  return "";
----------------
aemerson wrote:
> MatzeB wrote:
> > I assume this will be a correctness problem (on newer darwins?). So maybe we should not leave the decision to the frontend to set Options, but instead always make our decision based on the target triple (being darwin and newer than some version). Otherwise I can easily see non-clang frontends such as the various JITs we have around to miss the setting and fail.
> > 
> > You could then additionally provide a cl::opt then for cases where users want to explicitely disable the probe code generation for some reason.
> Akira suggested on D40864 that we use function attributes for this instead of a codegen option. That will still require front-ends to handle it. If we to unconditionally enable this in the backend based on target triple perhaps that would break cases where JITs etc don't automatically link in compiler-rt?
He's right that for users explicitely enabling/disabling the feature a function attribute would be best.

I think the question whether we want the logic in llvm or just in the frontend depends a bit on why we need/want stack probing.


Repository:
  rL LLVM

https://reviews.llvm.org/D40863