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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm marked 4 inline comments as done.
xiangzhangllvm added a comment.

TKS for reviewing!



================
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:
> nickdesaulniers wrote:
> > These are going to be target dependent, right?
> I don't think this was addressed.
Use string, let target deal with it now.


================
Comment at: clang/test/Driver/stack-protector-guard.c:18
+// Invalid arch
+// RUN: not %clang -target arm-eabi-c -mstack-protector-guard=tls %s 2>&1 | \
+// RUN:   FileCheck -check-prefix=INVALID-ARCH %s
----------------
Add non-x86 arch here before here and this time add following 2 checks.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:474-477
+  if (getStackProtectorGuardReg() == "fs")
+    return StackProtectorGuardRegs::Reg0;
+  if (getStackProtectorGuardReg() == "gs")
+    return StackProtectorGuardRegs::Reg1;
----------------
nickdesaulniers wrote:
> This is still target dependent. You need to check the target before checking target specific registers.
Done by directly passing string for GuardReg option.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:492
+codegen::getStackProtectorGuardMode(llvm::TargetOptions &Options) {
+  if (getStackProtectorGuard() == "tls")
+    return StackProtectorGuards::TLS;
----------------
nickdesaulniers wrote:
> Would a `StringSwitch` be more appropriate here?
It not convenient to hand to "none", and can I keep the same style with other functions in this file? because I didn't saw StringSwitch use in this file.


================
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;
----------------
nickdesaulniers wrote:
> Do we want to retain `Guard`'s previous scope?
I know your idea, I tried to change with following patch, but it got built fail.

```
-  Value *Guard = TLI->getIRStackGuard(B);
   auto GuardMode = TLI->getTargetMachine().Options.StackProtectorGuard;
-
-  if ((GuardMode == llvm::StackProtectorGuards::TLS ||
-       GuardMode == llvm::StackProtectorGuards::None) && Guard)
+  if ((Value *Guard = TLI->getIRStackGuard(B)) &&
+      (GuardMode == llvm::StackProtectorGuards::TLS ||
+       GuardMode == llvm::StackProtectorGuards::None))
```


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

https://reviews.llvm.org/D88631