nickdesaulniers created this revision.
nickdesaulniers added reviewers: void, manojgupta, rnk.
Herald added subscribers: cfe-commits, dexonsmith, pengfei.
Herald added a project: clang.
nickdesaulniers requested review of this revision.
Follow up to:
1. commit b7926ce6d7a8
<https://reviews.llvm.org/rGb7926ce6d7a83cdf70c68d82bc3389c04009b841> ("[IR]
add fn attr for no_stack_protector; prevent inlining on mismatch") which was a
fix for `__attribute__((no_stack_protector))` source code annotations, but not
`-fno-stack-protector` command line flags (which this patch addresses).
2. commit 0b11d018cc2f
<https://reviews.llvm.org/rG0b11d018cc2f2c6bea5dac8dc72140cdb502ca02>
("[BitCode] decode nossp fn attr") which was a small fixup for 1 above.
This patch does 3 things (and could be split up and landed sequentially
if needed):
1. Make the virtual method Toolchain::GetDefaultStackProtectorLevel() return an
explict enum value rather than an integral constant. This makes the code
subjectively easier to read, and should help prevent bugs that may (or may
never) arise from changing the enum values. Previously, these were just kept in
sync via a comment, which is brittle. The trade off is including a additional
header in a few new places. It is not necessary, but in my opinion helps the
readability.
2. The front end boils down the GCC/MSVC compatible flags into front end
agnostic `-stack-protector <N>` for cc1. `-stack-protector 0` and not
specifying the flag at all were previously handled in the same way. This made
it difficult to differentiate between code compiled explicitly with
`-fno-stack-protector` vs being the level of stack protection unspecified.
This patch adds a new enum value for the stack protector being unspecified,
and changes `-stack-protector 0` to always mean "we do not want a stack
protector." `-stack-protector <N>` is not passed to cc1 if the front end does
not specify a stack protection level (there's complex logic based on the target
what the stack protection level should be in that case, see the virtual
overrides of GetDefaultStackProtectorLevel). Where some overrides used to
return `0` to mean "don't care/unspecified," (which in other contexts
additionally meant "no stack protector, please") they now intentionally return
the new enum value meaning just "don't care/unspecified."
Adding the new enum value to the end is actually the smallest incision, at
the trivial cost of one new conditional check in RenderSSPOptions().
An alternative approach to consider would be not adding a new enum, and
making `-stack-protector` ALWAYS be specified when invoking cc1. I did not
quantify the amount of changes to existing tests, but am happy to do so if
reviewers would like.
3. `-fno-stack-protector` now explicitly sets `-stack-protector 0` for the
invocation of cc1. In turn, `-stack-protector 0` will set `nossp` IR fn attr on
all functions defined in the translation unit.
This now allows mixed use of translation units that specify
`-fno-stack-protector` with translation units with stronger stack protector
options to linked via [Thin]LTO, resolving pr/47479.
The ultimate test for that is building an x86_64 Linux kernel with LTO, and
verifying the disassembly of the function __restore_processor_state does not
contain a stack protector, since its translation unit was built with
`-fno-stack-protector`, which I have manually verified.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D90194
Files:
clang/include/clang/Basic/LangOptions.def
clang/include/clang/Basic/LangOptions.h
clang/include/clang/Driver/ToolChain.h
clang/lib/CodeGen/CodeGenModule.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Driver/ToolChains/CrossWindows.h
clang/lib/Driver/ToolChains/Darwin.h
clang/lib/Driver/ToolChains/Fuchsia.h
clang/lib/Driver/ToolChains/OpenBSD.h
clang/lib/Driver/ToolChains/PS4CPU.h
clang/lib/Frontend/CompilerInvocation.cpp
clang/test/CodeGen/stack-protector.c
clang/test/Driver/stack-protector.c
Index: clang/test/Driver/stack-protector.c
===================================================================
--- clang/test/Driver/stack-protector.c
+++ clang/test/Driver/stack-protector.c
@@ -1,6 +1,6 @@
// RUN: %clang -fno-stack-protector -### %s 2>&1 | FileCheck %s -check-prefix=NOSSP
-// NOSSP-NOT: "-stack-protector"
-// NOSSP-NOT: "-stack-protector-buffer-size"
+// NOSSP: "-stack-protector" "0"
+// NOSSP-NOT: "-stack-protector-buffer-size"
// RUN: %clang -target i386-unknown-linux -fstack-protector -### %s 2>&1 | FileCheck %s -check-prefix=SSP
// SSP: "-stack-protector" "1"
Index: clang/test/CodeGen/stack-protector.c
===================================================================
--- clang/test/CodeGen/stack-protector.c
+++ clang/test/CodeGen/stack-protector.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 0 | FileCheck -check-prefix=DEF -check-prefix=NOSSP %s
+// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 0 | FileCheck -check-prefix=DEF-NOSSP -check-prefix=NOSSP %s
// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 1 | FileCheck -check-prefix=DEF -check-prefix=SSP %s
// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 2 | FileCheck -check-prefix=DEF -check-prefix=SSPSTRONG %s
// RUN: %clang_cc1 -emit-llvm -o - %s -stack-protector 3 | FileCheck -check-prefix=DEF -check-prefix=SSPREQ %s
@@ -16,6 +16,7 @@
char *strcpy(char *s1, const char *s2);
// DEF: define {{.*}}void @test1(i8* %msg) #[[A:.*]] {
+// DEF-NOSSP: define {{.*}}void @test1(i8* %msg) #[[B:.*]] {
void test1(const char *msg) {
char a[strlen(msg) + 1];
strcpy(a, msg);
@@ -23,6 +24,7 @@
}
// DEF: define {{.*}}void @test2(i8* %msg) #[[B:.*]] {
+// DEF-NOSSP: define {{.*}}void @test2(i8* %msg) #[[B:.*]] {
__attribute__((no_stack_protector))
void test2(const char *msg) {
char a[strlen(msg) + 1];
Index: clang/lib/Frontend/CompilerInvocation.cpp
===================================================================
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3379,7 +3379,7 @@
Opts.RetainCommentsFromSystemHeaders =
Args.hasArg(OPT_fretain_comments_from_system_headers);
- unsigned SSP = getLastArgIntValue(Args, OPT_stack_protector, 0, Diags);
+ unsigned SSP = getLastArgIntValue(Args, OPT_stack_protector, UINT_MAX, Diags);
switch (SSP) {
default:
Diags.Report(diag::err_drv_invalid_value)
@@ -3389,6 +3389,9 @@
case 1: Opts.setStackProtector(LangOptions::SSPOn); break;
case 2: Opts.setStackProtector(LangOptions::SSPStrong); break;
case 3: Opts.setStackProtector(LangOptions::SSPReq); break;
+ case UINT_MAX:
+ Opts.setStackProtector(LangOptions::SSPUnspecified);
+ break;
}
if (Arg *A = Args.getLastArg(OPT_ftrivial_auto_var_init)) {
Index: clang/lib/Driver/ToolChains/PS4CPU.h
===================================================================
--- clang/lib/Driver/ToolChains/PS4CPU.h
+++ clang/lib/Driver/ToolChains/PS4CPU.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_PS4CPU_H
#include "Gnu.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"
@@ -73,8 +74,9 @@
bool HasNativeLLVMSupport() const override;
bool isPICDefault() const override;
- unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
- return 2; // SSPStrong
+ enum LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
+ return LangOptions::SSPStrong;
}
llvm::DebuggerKind getDefaultDebuggerTuning() const override {
Index: clang/lib/Driver/ToolChains/OpenBSD.h
===================================================================
--- clang/lib/Driver/ToolChains/OpenBSD.h
+++ clang/lib/Driver/ToolChains/OpenBSD.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_OPENBSD_H
#include "Gnu.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"
@@ -79,8 +80,9 @@
std::string getCompilerRT(const llvm::opt::ArgList &Args, StringRef Component,
FileType Type = ToolChain::FT_Static) const override;
- unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
- return 2;
+ LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
+ return LangOptions::SSPStrong;
}
unsigned GetDefaultDwarfVersion() const override { return 2; }
Index: clang/lib/Driver/ToolChains/Fuchsia.h
===================================================================
--- clang/lib/Driver/ToolChains/Fuchsia.h
+++ clang/lib/Driver/ToolChains/Fuchsia.h
@@ -10,6 +10,7 @@
#define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_FUCHSIA_H
#include "Gnu.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"
@@ -59,8 +60,9 @@
return llvm::DebuggerKind::GDB;
}
- unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
- return 2; // SSPStrong
+ LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
+ return LangOptions::SSPStrong;
}
std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
Index: clang/lib/Driver/ToolChains/Darwin.h
===================================================================
--- clang/lib/Driver/ToolChains/Darwin.h
+++ clang/lib/Driver/ToolChains/Darwin.h
@@ -491,17 +491,18 @@
return !(isTargetMacOS() && isMacosxVersionLT(10, 6));
}
- unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
+ LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
// Stack protectors default to on for user code on 10.5,
// and for everything in 10.6 and beyond
if (isTargetIOSBased() || isTargetWatchOSBased())
- return 1;
+ return LangOptions::SSPOn;
else if (isTargetMacOS() && !isMacosxVersionLT(10, 6))
- return 1;
+ return LangOptions::SSPOn;
else if (isTargetMacOS() && !isMacosxVersionLT(10, 5) && !KernelOrKext)
- return 1;
+ return LangOptions::SSPOn;
- return 0;
+ return LangOptions::SSPUnspecified;
}
void CheckObjCARC() const override;
Index: clang/lib/Driver/ToolChains/CrossWindows.h
===================================================================
--- clang/lib/Driver/ToolChains/CrossWindows.h
+++ clang/lib/Driver/ToolChains/CrossWindows.h
@@ -11,6 +11,7 @@
#include "Cuda.h"
#include "Gnu.h"
+#include "clang/Basic/LangOptions.h"
#include "clang/Driver/Tool.h"
#include "clang/Driver/ToolChain.h"
@@ -59,8 +60,9 @@
bool isPIEDefault() const override;
bool isPICDefaultForced() const override;
- unsigned int GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
- return 0;
+ LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
+ return LangOptions::SSPUnspecified;
}
void
Index: clang/lib/Driver/ToolChains/Clang.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2994,9 +2994,9 @@
if (EffectiveTriple.isNVPTX())
return;
- // -stack-protector=0 is default.
- unsigned StackProtectorLevel = 0;
- unsigned DefaultStackProtectorLevel =
+ LangOptions::StackProtectorMode StackProtectorLevel =
+ LangOptions::SSPUnspecified;
+ LangOptions::StackProtectorMode DefaultStackProtectorLevel =
TC.GetDefaultStackProtectorLevel(KernelOrKext);
if (Arg *A = Args.getLastArg(options::OPT_fno_stack_protector,
@@ -3004,17 +3004,22 @@
options::OPT_fstack_protector_strong,
options::OPT_fstack_protector)) {
if (A->getOption().matches(options::OPT_fstack_protector))
- StackProtectorLevel =
- std::max<unsigned>(LangOptions::SSPOn, DefaultStackProtectorLevel);
+ if (DefaultStackProtectorLevel == LangOptions::SSPUnspecified)
+ StackProtectorLevel = LangOptions::SSPOn;
+ else
+ StackProtectorLevel =
+ std::max<>(LangOptions::SSPOn, DefaultStackProtectorLevel);
else if (A->getOption().matches(options::OPT_fstack_protector_strong))
StackProtectorLevel = LangOptions::SSPStrong;
else if (A->getOption().matches(options::OPT_fstack_protector_all))
StackProtectorLevel = LangOptions::SSPReq;
+ else if (A->getOption().matches(options::OPT_fno_stack_protector))
+ StackProtectorLevel = LangOptions::SSPOff;
} else {
StackProtectorLevel = DefaultStackProtectorLevel;
}
- if (StackProtectorLevel) {
+ if (StackProtectorLevel != LangOptions::SSPUnspecified) {
CmdArgs.push_back("-stack-protector");
CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1594,7 +1594,8 @@
if (!hasUnwindExceptions(LangOpts))
B.addAttribute(llvm::Attribute::NoUnwind);
- if (D && D->hasAttr<NoStackProtectorAttr>())
+ if (LangOpts.getStackProtector() == LangOptions::SSPOff ||
+ (D && D->hasAttr<NoStackProtectorAttr>()))
B.addAttribute(llvm::Attribute::NoStackProtect);
else if (LangOpts.getStackProtector() == LangOptions::SSPOn)
B.addAttribute(llvm::Attribute::StackProtect);
Index: clang/include/clang/Driver/ToolChain.h
===================================================================
--- clang/include/clang/Driver/ToolChain.h
+++ clang/include/clang/Driver/ToolChain.h
@@ -382,9 +382,10 @@
virtual bool useRelaxRelocations() const;
/// GetDefaultStackProtectorLevel - Get the default stack protector level for
- /// this tool chain (0=off, 1=on, 2=strong, 3=all).
- virtual unsigned GetDefaultStackProtectorLevel(bool KernelOrKext) const {
- return 0;
+ /// this tool chain.
+ virtual LangOptions::StackProtectorMode
+ GetDefaultStackProtectorLevel(bool KernelOrKext) const {
+ return LangOptions::SSPUnspecified;
}
/// Get the default trivial automatic variable initialization.
Index: clang/include/clang/Basic/LangOptions.h
===================================================================
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -57,7 +57,7 @@
using RoundingMode = llvm::RoundingMode;
enum GCMode { NonGC, GCOnly, HybridGC };
- enum StackProtectorMode { SSPOff, SSPOn, SSPStrong, SSPReq };
+ enum StackProtectorMode { SSPOff, SSPOn, SSPStrong, SSPReq, SSPUnspecified };
// Automatic variables live on the stack, and when trivial they're usually
// uninitialized because it's undefined behavior to use them without
Index: clang/include/clang/Basic/LangOptions.def
===================================================================
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -309,7 +309,7 @@
"apply global symbol visibility to external declarations without an explicit visibility")
BENIGN_LANGOPT(SemanticInterposition , 1, 0, "semantic interposition")
BENIGN_LANGOPT(ExplicitNoSemanticInterposition, 1, 0, "explicitly no semantic interposition")
-ENUM_LANGOPT(StackProtector, StackProtectorMode, 2, SSPOff,
+ENUM_LANGOPT(StackProtector, StackProtectorMode, 3, SSPUnspecified,
"stack protector mode")
ENUM_LANGOPT(TrivialAutoVarInit, TrivialAutoVarInitKind, 2, TrivialAutoVarInitKind::Uninitialized,
"trivial automatic variable initialization")
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
