[PATCH] D78717: [SystemZ] Implement -fstack-clash-protection

Mon May 18 02:38:17 PDT 2020

jonpa marked 4 inline comments as done.
jonpa added inline comments.

================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:670
+
+  if (NumFullBlocks < 3) {
+    // Emit unrolled probe statements.
----------------
serge-sans-paille wrote:
> Maybe we should syndicate that unroll parameter somewhere across architectures?
I am not sure I see the benefit of that since this entire method is already defined by the target. 

================
Comment at: llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp:708
+  if (Residual)
+    allocateAndProbe(*MBB, MBBI, Residual, true/*EmitCFI*/);
+
----------------
serge-sans-paille wrote:
> On X86, we don't need to probe that final allocation because the two ways the stack could grow after the final alloca are
> 
> 1. A function call, and in that case we get a free probe when we make a function call
> 2. A PROBED_ALLOCA, and in that case we get a probe at Residual + PAGE_SIZE, which is right into the Page Guard.
> 
> I assume that's different for SystemZ? 
There's not always a free probe with a function call on SystemZ (only if stack space is allocated by the called function), but regardless of that I think the residual is needed. Since the probe is done on the topmost byte of each allocated block on SystemZ, the guard page could fit within the last allocated full block and a residual of just 8 or more bytes (probing the high byte is what GCC is doing on SystemZ).

I still wonder if that would not be necessary also on X86 in case "2": Given that you on X86 probe the lower byte of each full block, you could get a residual into the guard page, and then if the dynamic alloca started with a full block, the next probe would not be into the guard page, but past it:

```
|     |     |GGGGG|     |
     |    P|   R|    P|

```

Maybe I am missing something?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78717/new/

https://reviews.llvm.org/D78717