Page MenuHomePhabricator

[Driver] split LangOptions::SSPOff into SSPOFF and SSPUnspecified
AbandonedPublic

Authored by nickdesaulniers on Oct 26 2020, 2:44 PM.

Details

Summary

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.

Where some overrides used to return SSPOff 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."

This permits a follow up change to have -fno-stack-protector//GS_
mean "no stack protectors" as opposed to "don't care."

Diff Detail

Unit TestsFailed

TimeTest
390 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2020, 2:44 PM
nickdesaulniers requested review of this revision.Oct 26 2020, 2:44 PM

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
nickdesaulniers planned changes to this revision.Oct 27 2020, 3:39 PM

I split out the first logical change into https://reviews.llvm.org/D90271. I think I'll rework this to both be rebased, and to not add the new enum value.

It's trivial to not create a new enum value and always pass -stack-protector <N> along

That was incorrect. Rebasing this on top of D90271 causes 49 test failures, because the diff I posted above didn't actually remove the new enum or changes to clang/lib/Frontend/CompilerInvocation.cpp. So I've answered my own question; we should add this enum value. Let me rebase this differently then.

nickdesaulniers retitled this revision from [Driver] differentiate -stack-protector 0 from being unspecified to [Driver] split LangOptions::SSPOff into SSPOFF and SSPUnspecified.Oct 28 2020, 3:36 PM
nickdesaulniers edited the summary of this revision. (Show Details)
nickdesaulniers edited the summary of this revision. (Show Details)

Bumping for review

void added inline comments.Nov 4 2020, 5:43 PM
clang/include/clang/Basic/LangOptions.h
60

I don't know if it's a convention, but an unspecified enum is typically put at the beginning of the enum list.

nickdesaulniers added inline comments.Nov 4 2020, 6:00 PM
clang/include/clang/Basic/LangOptions.h
60

This ordering produces the least amount of churn. If I change the ordering, then all of the enum values need to be incremented across all unit tests that specify -stack-protector <N> to cc1. It would simplify the use of std::max in RenderSSPOptions slightly. WDYT?

rnk added a comment.Nov 5 2020, 1:53 PM

Why does -cc1 need to distinguish between enabled, disabled, unset? The design philosophy is that the driver figures out all the target-specific configuration stuff, and then tells cc1 which features to enable. See, for example, -fexceptions, which is off by default in cc1, and removed by -fno-exceptions.

rnk added a comment.Nov 5 2020, 1:58 PM

Given that only three tests fail when the nossp attribute gets added from -cc1 with no stack protector, I think it's reasonable to add it and skip adding the extra enum.

I think it would be weird if we LTO'd together three kinds of objects (sp on, sp off, sp unspecified) and the inliner was able to inline some of them. It seems better to decide that unspecified either means on or off, depending on the target defaults.

In D90194#2377337, @rnk wrote:

Given that only three tests fail when the nossp attribute gets added from -cc1 with no stack protector, I think it's reasonable to add it and skip adding the extra enum.

Sorry, when I said:

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:

I followed up that it wasn't as simple as 3 tests failing. I followed up with:

It's trivial to not create a new enum value and always pass -stack-protector <N> along

That was incorrect. Rebasing this on top of D90271 causes 49 test failures, because the diff I posted above didn't actually remove the new enum or changes to clang/lib/Frontend/CompilerInvocation.cpp. So I've answered my own question; we should add this enum value. Let me rebase this differently then.

I'm happy to rewrite this patch to not introduce the new enum, but it's going to be much larger than this patch due to churn in the tests. Lets me take a crack at it and I'll post it as a separate review. Then I think it would help illuminate the differences in approach and we can take our pick. Thanks all for the feedback.

D90896 is the alternative solution that doesn't add any new enumeration values.

void added inline comments.Thu, Nov 12, 2:10 PM
clang/include/clang/Basic/LangOptions.h
60

Maybe something like this?

enum StackProtectorMode {
    SSPUnspecified = -1,
    SSPOff = 0,
    SSPOn,
    SSPStrong,
    SSPReq
};