[PATCH] D90194: [Driver] differentiate -stack-protector 0 from being unspecified

[Nick Desaulniers via Phabricator via cfe-commits]

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