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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:2245
 def mcmodel_EQ : Joined<["-"], "mcmodel=">, Group<m_Group>, Flags<[CC1Option]>;
+def mstack_protector_guard_EQ : Joined<["-"], "mstack-protector-guard=">, Group<m_Group>, Flags<[DriverOption, CC1Option]>,
+  HelpText<"Use the given guard (global, tls) for addressing the stack-protector guard.">;
----------------
You can drop `DriverOption` (clang::driver::options::DriverOption bit). It is mostly useless now. Ditto below


================
Comment at: clang/include/clang/Driver/Options.td:2246
+def mstack_protector_guard_EQ : Joined<["-"], "mstack-protector-guard=">, Group<m_Group>, Flags<[DriverOption, CC1Option]>,
+  HelpText<"Use the given guard (global, tls) for addressing the stack-protector guard.">;
 def mtls_size_EQ : Joined<["-"], "mtls-size=">, Group<m_Group>, Flags<[DriverOption, CC1Option]>,
----------------
No period in the help message


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3029
+      D.Diag(diag::err_drv_invalid_value) << A->getOption().getName() << Value;
+    CmdArgs.push_back(Args.MakeArgString("-mstack-protector-guard-reg=" +
+                                         Value));
----------------
`A->render(Args, CmdArgs)`


================
Comment at: clang/test/Driver/stack-protector-guard.c:1
+// RUN: %clang -### -target x86_64-linux-gnu -mstack-protector-guard=tls %s 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-TLS %s
----------------
`x86_64` if it works for other ELF x86_64 OSes.


================
Comment at: clang/test/Driver/stack-protector-guard.c:7
+// Invalid option value
+// RUN: not %clang -target x86_64-linux-gnu -mstack-protector-guard=zx %s 2>&1 \
+// RUN: | FileCheck -check-prefix=INVALID-VALUE %s
----------------
For `not %clang` tests, it is best to add a -c to avoid the linker stage.


================
Comment at: clang/test/Driver/stack-protector-guard.c:15
+// RUN: %clang -### -target x86_64-linux-gnu -mstack-protector-guard-reg=fs %s 2>&1 \
+// RUN: | FileCheck -check-prefix=CHECK-FS %s
+// RUN: %clang -### -target x86_64-linux-gnu -mstack-protector-guard-reg=gs %s 2>&1 \
----------------
The prevailing form is to place `| \` at the end and indent the continuation line by 2.




================
Comment at: llvm/include/llvm/CodeGen/CommandFlags.h:99
 
+std::string getStackProtectorGuard();
+
----------------
These related declarations do not need empty lines


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:77
+  enum class StackProtectorGuards {
+    TLS,
+    Global,
----------------
None as the first may be what most users would expect.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:476
+    return StackProtectorGuardRegs::FS;
+  else if (getStackProtectorGuardReg() == "gs")
+    return StackProtectorGuardRegs::GS;
----------------
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:484
+    if (!MBOrErr) {
+      errs() << "Error illegal stack protector guard register: "
+             << MBOrErr.getError().message() << "\n";
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:497
+    return StackProtectorGuards::TLS;
+  else if (getStackProtectorGuard() == "global")
+    return StackProtectorGuards::Global;
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


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

https://reviews.llvm.org/D88631