[PATCH] D88631: [X86] Support customizing stack protector guard
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 30 22:42:51 PDT 2020
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
More information about the llvm-commits
mailing list