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

[nickdesaulniers]

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."

[nickdesaulniers]

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]

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.

[nickdesaulniers]

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]

• rebase on D90271

[nickdesaulniers]

Bumping for review

[void]

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]

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]

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]

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.

[nickdesaulniers]

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:

In D90194#2354908, @nickdesaulniers wrote:

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:

In D90194#2360392, @nickdesaulniers wrote:

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.

[nickdesaulniers]

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

[void]

clang/include/clang/Basic/LangOptions.h

60

Maybe something like this?

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

[nickdesaulniers]

Prefer: https://reviews.llvm.org/D91816

clang/include/clang/Basic/LangOptions.h

 /// Keeps track of the various options that can be
 /// enabled, which controls the dialect of C or C++ that is accepted.
 class LangOptions : public LangOptionsBase {
 public:
   using Visibility = clang::Visibility;
   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
   // initializing them.
   enum class TrivialAutoVarInitKind { Uninitialized, Zero, Pattern };
 
   enum SignedOverflowBehaviorTy {
     // Default C standard behavior.

clang/include/clang/Basic/LangOptions.def

 ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,
              "default visibility for functions and variables [-fvisibility]")
 ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
              "default visibility for types [-ftype-visibility]")
 LANGOPT(SetVisibilityForExternDecls, 1, 0,
         "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")
 VALUE_LANGOPT(TrivialAutoVarInitStopAfter, 32, 0,
              "stop trivial automatic variable initialization after the specified number of instances. Must be greater than 0.")
 ENUM_LANGOPT(SignedOverflowBehavior, SignedOverflowBehaviorTy, 2, SOB_Undefined,
              "signed integer overflow handling")
 

clang/include/clang/Driver/ToolChain.h

public:
 
   /// Check whether to enable x86 relax relocations by default.
   virtual bool useRelaxRelocations() const;
 
   /// GetDefaultStackProtectorLevel - Get the default stack protector level for
   /// this tool chain.
   virtual LangOptions::StackProtectorMode
   GetDefaultStackProtectorLevel(bool KernelOrKext) const {
-    return LangOptions::SSPOff;
+    return LangOptions::SSPUnspecified;
   }
 
   /// Get the default trivial automatic variable initialization.
   virtual LangOptions::TrivialAutoVarInitKind
   GetDefaultTrivialAutoVarInit() const {
     return LangOptions::TrivialAutoVarInitKind::Uninitialized;
   }
 

clang/lib/Driver/ToolChains/Clang.cpp

static void RenderSSPOptions(const Driver &D, const ToolChain &TC,
                              bool KernelOrKext) {
   const llvm::Triple &EffectiveTriple = TC.getEffectiveTriple();
 
   // NVPTX doesn't support stack protectors; from the compiler's perspective, it
   // doesn't even have a stack!
   if (EffectiveTriple.isNVPTX())
     return;
 
-  // -stack-protector=0 is default.
-  LangOptions::StackProtectorMode StackProtectorLevel = LangOptions::SSPOff;
+  LangOptions::StackProtectorMode StackProtectorLevel =
+      LangOptions::SSPUnspecified;
   LangOptions::StackProtectorMode DefaultStackProtectorLevel =
       TC.GetDefaultStackProtectorLevel(KernelOrKext);
 
   if (Arg *A = Args.getLastArg(options::OPT_fno_stack_protector,
                                options::OPT_fstack_protector_all,
                                options::OPT_fstack_protector_strong,
                                options::OPT_fstack_protector)) {
     if (A->getOption().matches(options::OPT_fstack_protector))
+      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 {
     StackProtectorLevel = DefaultStackProtectorLevel;
   }
 
-  if (StackProtectorLevel) {
+  if (StackProtectorLevel != LangOptions::SSPUnspecified) {
     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)) {
     StringRef Str(A->getValue());
     if (Str.startswith("ssp-buffer-size=")) {

clang/lib/Driver/ToolChains/CrossWindows.h

public:
   bool IsIntegratedAssemblerDefault() const override { return true; }
   bool IsUnwindTablesDefault(const llvm::opt::ArgList &Args) const override;
   bool isPICDefault() const override;
   bool isPIEDefault() const override;
   bool isPICDefaultForced() const override;
 
   LangOptions::StackProtectorMode
   GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
-    return LangOptions::SSPOff;
+    return LangOptions::SSPUnspecified;
   }
 
   void
   AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                             llvm::opt::ArgStringList &CC1Args) const override;
   void AddClangCXXStdlibIncludeArgs(
       const llvm::opt::ArgList &DriverArgs,
       llvm::opt::ArgStringList &CC1Args) const override;

clang/lib/Driver/ToolChains/Darwin.h

GetDefaultStackProtectorLevel(bool KernelOrKext) const override {
     // and for everything in 10.6 and beyond
     if (isTargetIOSBased() || isTargetWatchOSBased())
       return LangOptions::SSPOn;
     else if (isTargetMacOS() && !isMacosxVersionLT(10, 6))
       return LangOptions::SSPOn;
     else if (isTargetMacOS() && !isMacosxVersionLT(10, 5) && !KernelOrKext)
       return LangOptions::SSPOn;
 
-    return LangOptions::SSPOff;
+    return LangOptions::SSPUnspecified;
   }
 
   void CheckObjCARC() const override;
 
   llvm::ExceptionHandling GetExceptionModel(
       const llvm::opt::ArgList &Args) const override;
 
   bool SupportsEmbeddedBitcode() const override;

clang/lib/Frontend/CompilerInvocation.cpp

if (Arg *A = Args.getLastArg(OPT_ffp_exception_behavior_EQ)) {
     else
       Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
   Opts.setFPExceptionMode(FPEB);
 
   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)
       << Args.getLastArg(OPT_stack_protector)->getAsString(Args) << SSP;
     break;
   case 0: Opts.setStackProtector(LangOptions::SSPOff); break;
   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)) {
     StringRef Val = A->getValue();
     if (Val == "uninitialized")
       Opts.setTrivialAutoVarInit(
           LangOptions::TrivialAutoVarInitKind::Uninitialized);
     else if (Val == "zero")

clang/test/CodeGen/stack-protector.c

-// 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
 
 // RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-NOSSP %s
 // RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-protector 0 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-NOSSP %s
 // RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-protector 1 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-SSP %s
 // RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-protector 2 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-SSPSTRONG %s
 // RUN: %clang_cc1 -emit-llvm -o - %s -fsanitize=safe-stack -stack-protector 3 | FileCheck -check-prefix=DEF -check-prefix=SAFESTACK-SSPREQ %s
 
 typedef __SIZE_TYPE__ size_t;
 
 int printf(const char * _Format, ...);
 size_t strlen(const char *s);
 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);
   printf("%s\n", a);
 }
 
 // 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];
   strcpy(a, msg);
   printf("%s\n", a);
 }
 
 // NOSSP-NOT: attributes #[[A]] = {{.*}} ssp