[PATCH] D68720: Support -fstack-clash-protection for x86

[serge via Phabricator via llvm-commits]

serge-sans-paille marked 3 inline comments as done.
serge-sans-paille added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:400
+        !(STI.isOSWindows() && !STI.isTargetMachO());
+    if (InlineStackClashProtector && !InEpilogue) {
+      const uint64_t PageSize = TLI.getStackProbeSize(MF);
----------------
efriedma wrote:
> Why is this code in a different location from the stack probing code that generates a call?
Because `BuildStackAdjustment` has more callsites than just emitPrologue and we want to capture all stack manipulation.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:408
+        CurrentAbsOffset += ChunkSize;
+        MI->getOperand(3).setIsDead(); // The EFLAGS implicit def is dead.
+
----------------
efriedma wrote:
> This algorithm needs some documentation; it isn't at all obvious what it's doing.  Particularly the interaction with "free" stack probes.
> 
> Should we generate a loop if the stack frame is large?
> Should we generate a loop if the stack frame is large?

That's an option. it's more complex to make looping compatible with free probes, but that's possible.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:408
+        CurrentAbsOffset += ChunkSize;
+        MI->getOperand(3).setIsDead(); // The EFLAGS implicit def is dead.
+
----------------
serge-sans-paille wrote:
> efriedma wrote:
> > This algorithm needs some documentation; it isn't at all obvious what it's doing.  Particularly the interaction with "free" stack probes.
> > 
> > Should we generate a loop if the stack frame is large?
> > Should we generate a loop if the stack frame is large?
> 
> That's an option. it's more complex to make looping compatible with free probes, but that's possible.
> This algorithm needs some documentation
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720