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

[serge via Phabricator via llvm-commits]

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


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2910
+
+  if (Arg *A = Args.getLastArg(options::OPT_fnostack_clash_protection,
+                               options::OPT_fstack_clash_protection)) {
----------------
craig.topper wrote:
> I think this can use Args.hasFlag(options::OPT_fstack_clash_protection, options::OPT_fnostack_clash_protection, false) and then you don't need the 'if'
I wasn't aware of that API, thanks for pointing this out o/


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31080
+
+  BuildMI(blockMBB, DL, TII->get(IsLP64 ? X86::SUB64ri32 : X86::SUB32ri),
+          physSPReg)
----------------
craig.topper wrote:
> This uses physSPReg, but doesn't match the condition for when physSPReg is a 64-bit register. Same at several places below.
Sorry, I don't understand your remark. Can you elaborate?


================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:130
+                         (X86ProbedAlloca GR64:$size))]>,
+                    Requires<[In64BitMode]>;
 }
----------------
craig.topper wrote:
> Why is this In64BitMode, but above is NotLP64. Shouldn't they be opposites? Looks like this was just copied from SEG_ALLOCA above?
Yeah, these intrinsics are just the same as the one above, except they have a different name. And they get lowered differently. I'm not familiar with that part of LLVM, can you suggest a better approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720