[PATCH] D88631: [X86] Support customizing stack protector guard

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the patch!  This is useful feature that allows for better protections; rather than having a single stack canary, it's now possible to have dynamic canary values.  The Linux kernel uses this, though only for 2 non-x86 architectures at the moment; Aarch64 and PowerPC.  Looking at its usage, here's my feedback for this patch:

1. the `-mstack-protector-guard-reg=` needs to consider the target.  As such if I use a different target, I shouldn't be able to specify x86 registers.  There should be an error emitted, and test coverage added in this patch for that. (clang/test/Driver/stack-protector-guard.c has one test for unsupported reg.  I would like to see a test for an x86 reg but Aarch64 target for example.  That would ensure our frontend validation isn't blindly accepting x86 specific reg values).
2. GCC also supports `sysreg` option for `-mstack-protector-guard=`, and it is used for Aarch64.  It would be good to add support for that option here in the parsing, even if there's no sane values for x86 for the corresponding `-mstack-protector-guard-reg=`.
3. There should be tests added and an error message for using these flags with unsupported architectures.  I don't expect you to implement support for the various targets, but I would appreciate testing that they fail with a reasonable error message saying these options aren't implemented for the current target.  I'd say this differs subtly from `1.` in that `1.` is more about regs for the wrong target, while `3.` is more generally about the target being unsupported.



================
Comment at: clang/include/clang/Driver/Options.td:2600
+def mstack_protector_guard_EQ : Joined<["-"], "mstack-protector-guard=">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Use the given guard (global, tls) for addressing the stack-protector guard">;
+def mstack_protector_guard_offset_EQ : Joined<["-"], "mstack-protector-guard-offset=">, Group<m_Group>, Flags<[CC1Option]>,
----------------
It looks like `-mstack-protector-guard=sysreg` is also supported by GCC.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:527
+          .Case("tls", llvm::StackProtectorGuards::TLS)
+          .Case("global", llvm::StackProtectorGuards::Global)
+          .Default(llvm::StackProtectorGuards::None);
----------------
need a `sysreg` option, too.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:533-534
+          .StackProtectorGuardReg)
+          .Case("fs", llvm::StackProtectorGuardRegs::FS)
+          .Case("gs", llvm::StackProtectorGuardRegs::GS)
+          .Default(llvm::StackProtectorGuardRegs::None);
----------------
These are going to be target dependent, right?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3006-3007
+
+  Arg * A = Args.getLastArg(options::OPT_mstack_protector_guard_EQ);
+  if (A) {
+    StringRef Value = A->getValue();
----------------
If this value of `A` is not reused post if statement, can these be combined and still fit on one line?

    if (Arg *A = ...)

Also the `*` should be against the `A`, no space.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3030
+    StringRef Value = A->getValue();
+    if (Value != "fs" && Value != "gs") {
+      D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
----------------
Arch specific?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3031
+    if (Value != "fs" && Value != "gs") {
+      D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
+      return;
----------------
Can we print something more helpful, like the list of supported values based on the target?


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:83
+  enum class StackProtectorGuardRegs {
+    FS,
+    GS,
----------------
xiangzhangllvm wrote:
> craig.topper wrote:
> > These register names are X86 specific. Should we be storing a string and let the relevant backend deal with it?
> I find all the options use unsigned or enum type.
> Can we rename it to {None, Reg0, Reg1} here? 
> if other target want support it, it can understand by itself.
I agree with @craig.topper here.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:2513
+  if ((GuardMode != llvm::StackProtectorGuards::Global)
+      && hasStackGuardSlotTLS(Subtarget.getTargetTriple()))
     return;
----------------
remove unnecessary extra parens


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

https://reviews.llvm.org/D88631