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

[Xiang Zhang via Phabricator via llvm-commits]

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

@Craig and MaskRay , thank you very much for your careful reviews !!



================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:360
+/// The default stack protector guard offset to use.
+VALUE_CODEGENOPT(StackProtectorGuardOffset, 32, 40)
+
----------------
craig.topper wrote:
> If this defaults to 40 won't that get passed to the backend and override the -1 check in X86TargetLowering::getIRStackGuard that is picks different values for 32 and 64 bit?
Yes, here if we don't set -mstack-protector-guard-offset=xxx, X86TargetLowering::getIRStackGuard will use value according to 32/64bits


================
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;
----------------
craig.topper wrote:
> The enum also has SS in it. Should we allow it here?
I find gcc don't use ss, let's sync.


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:83
+  enum class StackProtectorGuardRegs {
+    FS,
+    GS,
----------------
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.


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

https://reviews.llvm.org/D88631