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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

I would like to see a test added for a non x86 architecture, from the previous round of comments:

> 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/Basic/CodeGenOptions.h:330-332
+  std::string StackProtectorGuard;
+
+  std::string StackProtectorGuardReg;
----------------
Might be nice to have comments for these members?


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:533-534
+          .StackProtectorGuardReg)
+          .Case("fs", llvm::StackProtectorGuardRegs::FS)
+          .Case("gs", llvm::StackProtectorGuardRegs::GS)
+          .Default(llvm::StackProtectorGuardRegs::None);
----------------
nickdesaulniers wrote:
> These are going to be target dependent, right?
I don't think this was addressed.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2962
   const llvm::Triple &EffectiveTriple = TC.getEffectiveTriple();
+  const std::string &TripleStr = EffectiveTriple.getTriple();
 
----------------
Please sink this definition closer to its use below.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:474-477
+  if (getStackProtectorGuardReg() == "fs")
+    return StackProtectorGuardRegs::Reg0;
+  if (getStackProtectorGuardReg() == "gs")
+    return StackProtectorGuardRegs::Reg1;
----------------
This is still target dependent. You need to check the target before checking target specific registers.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:492
+codegen::getStackProtectorGuardMode(llvm::TargetOptions &Options) {
+  if (getStackProtectorGuard() == "tls")
+    return StackProtectorGuards::TLS;
----------------
Would a `StringSwitch` be more appropriate here?


================
Comment at: llvm/lib/CodeGen/StackProtector.cpp:384
                             bool *SupportsSelectionDAGSP = nullptr) {
-  if (Value *Guard = TLI->getIRStackGuard(B))
+  Value *Guard = TLI->getIRStackGuard(B);
+  auto GuardMode = TLI->getTargetMachine().Options.StackProtectorGuard;
----------------
Do we want to retain `Guard`'s previous scope?


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

https://reviews.llvm.org/D88631