nickdesaulniers added a comment.
> 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.
Making `-stack-protector` always be passed, and default to `0` rather than
being unspecified causes the following tests to fail:
Failed Tests (3):
Clang :: Driver/cl-options.c
Clang :: Driver/fsanitize.c
Clang :: Driver/stack-protector.c
which is much less than I was expecting. It's trivial to not create a new enum
value and always pass `-stack-protector <N>` along. On top of this patch:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp
b/clang/lib/Driver/ToolChains/Clang.cpp
index e6654940e9c0..43913c1daeb7 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3019,10 +3019,10 @@ static void RenderSSPOptions(const Driver &D, const
ToolChain &TC,
StackProtectorLevel = DefaultStackProtectorLevel;
}
- if (StackProtectorLevel != LangOptions::SSPUnspecified) {
- CmdArgs.push_back("-stack-protector");
- CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
- }
+ if (StackProtectorLevel == LangOptions::SSPUnspecified)
+ StackProtectorLevel = LangOptions::SSPOff;
+ CmdArgs.push_back("-stack-protector");
+ CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
// --param ssp-buffer-size=
for (const Arg *A : Args.filtered(options::OPT__param)) {
diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 89dbdebbaf69..8ace966b4f86 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -90,7 +90,7 @@
// GS: "-stack-protector" "2"
// RUN: %clang_cl /GS- -### -- %s 2>&1 | FileCheck -check-prefix=GS_ %s
-// GS_-NOT: -stack-protector
+// GS_: "-stack-protector" "0"
// RUN: %clang_cl /Gy -### -- %s 2>&1 | FileCheck -check-prefix=Gy %s
// Gy: -ffunction-sections
diff --git a/clang/test/Driver/fsanitize.c b/clang/test/Driver/fsanitize.c
index 0ecf656f292c..a6ece636dd15 100644
--- a/clang/test/Driver/fsanitize.c
+++ b/clang/test/Driver/fsanitize.c
@@ -673,12 +673,11 @@
// RUN: %clang -target arm-linux-androideabi -fsanitize=safe-stack -### %s
2>&1 | FileCheck %s -check-prefix=NO-SP
// RUN: %clang -target aarch64-linux-android -fsanitize=safe-stack -### %s
2>&1 | FileCheck %s -check-prefix=NO-SP
// RUN: %clang -target i386-contiki-unknown -fsanitize=safe-stack -### %s
2>&1 | FileCheck %s -check-prefix=NO-SP
-// NO-SP-NOT: stack-protector
// NO-SP: "-fsanitize=safe-stack"
// SP-ASAN: error: invalid argument '-fsanitize=safe-stack' not allowed with
'-fsanitize=address'
// SP: "-fsanitize=safe-stack"
// SP: -stack-protector
-// NO-SP-NOT: stack-protector
+// NO-SP: "-stack-protector" "0"
// RUN: %clang -target powerpc64-unknown-linux-gnu -fsanitize=memory %s -###
2>&1 | FileCheck %s -check-prefix=CHECK-SANM
// RUN: %clang -target powerpc64le-unknown-linux-gnu -fsanitize=memory %s
-### 2>&1 | FileCheck %s -check-prefix=CHECK-SANM
diff --git a/clang/test/Driver/stack-protector.c
b/clang/test/Driver/stack-protector.c
index 8366b2b00785..44affc636bbc 100644
--- a/clang/test/Driver/stack-protector.c
+++ b/clang/test/Driver/stack-protector.c
@@ -48,7 +48,7 @@
// RUN: %clang -ffreestanding -target x86_64-apple-darwin10
-mmacosx-version-min=10.5 -### %s 2>&1 | FileCheck %s
-check-prefix=SSP_MACOSX_10_5
// SSP_MACOSX_10_5: "-stack-protector" "1"
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.5
-mkernel -### %s 2>&1 | FileCheck %s -check-prefix=SSP_MACOSX_KERNEL
-// SSP_MACOSX_KERNEL-NOT: "-stack-protector"
+// SSP_MACOSX_KERNEL: "-stack-protector" "0"
// RUN: %clang -target x86_64-apple-darwin10 -mmacosx-version-min=10.6 -###
%s 2>&1 | FileCheck %s -check-prefix=SSP_MACOSX_10_6_KERNEL
// RUN: %clang -ffreestanding -target x86_64-apple-darwin10
-mmacosx-version-min=10.6 -### %s 2>&1 | FileCheck %s
-check-prefix=SSP_MACOSX_10_6_KERNEL
// SSP_MACOSX_10_6_KERNEL: "-stack-protector" "1"
The question is: the semantics of `nossp` fn attr in IR means that `nossp`
won't be inlined into `ssp`/`sspstrong`/`sspreq` callers and vice versa, so
should these targets (older OSX, Windows) attribute functions in such a way
that these functions cannot be inlined into functions with stack protectors (if
that ever occurs in practice outside of this unusual Linux kernel LTO issue)?
If yes, then I should update this patch to not use the new enum. If no, then
this patch is good to go. I'm leaning yes/doesn't matter.
> 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.
Do reviewers have thoughts on that:
1. plz no
2. do it
3. do it in a separate patch/precommit
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90194/new/
https://reviews.llvm.org/D90194
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
