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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:243
+
+  def warn_fe_stack_clash_protection_inline_asm : Warning<
+      "Unable to protect inline asm that clobbers stack pointer against stack clash">;
----------------
Remove "fe_" from this? Looks like thats only there because this used to be DiagnosticFrontendKinds?


================
Comment at: clang/lib/Basic/Targets/X86.h:169
 
+  const char *getSPRegName() const override {
+    if (getTriple().getArch() == llvm::Triple::x86)
----------------
This probably doesn't work correctly with X32 which is 64-bit but uses 32-bit pointers. Maybe we change this to "bool isSPReg(StringRef Reg)" and pass the register name to it so you can just check both strings always.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2910
+
+  if (Arg *A = Args.getLastArg(options::OPT_fnostack_clash_protection,
+                               options::OPT_fstack_clash_protection)) {
----------------
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'


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


================
Comment at: llvm/lib/Target/X86/X86InstrCompiler.td:130
+                         (X86ProbedAlloca GR64:$size))]>,
+                    Requires<[In64BitMode]>;
 }
----------------
Why is this In64BitMode, but above is NotLP64. Shouldn't they be opposites? Looks like this was just copied from SEG_ALLOCA above?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68720