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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3011
+      D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
+    CmdArgs.push_back(Args.MakeArgString("-mstack-protector-guard=" +
+                                         Value));
----------------
Probably shouldn't push the arg if its not valid.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3019
+    unsigned Offset;
+    Value.getAsInteger(10, Offset);
+    CmdArgs.push_back(Args.MakeArgString("-mstack-protector-guard-offset=" +
----------------
Verify that this parsed correctly?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3027
+    StringRef Value = A->getValue();
+    if (Value != "fs" && Value != "gs")
+      D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
----------------
The enum also has SS in it. Should we allow it here?


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:83
+  enum class StackProtectorGuardRegs {
+    FS,
+    GS,
----------------
These register names are X86 specific. Should we be storing a string and let the relevant backend deal with it?


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:258
 
+    /// Stack protector guard mode to use: fs or gs.
+    StackProtectorGuards StackProtectorGuard =
----------------
"fs or gs" should be global or tls right?


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

https://reviews.llvm.org/D88631