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

Mon Oct 26 14:44:37 PDT 2020

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D90194.300798.patch
Type: text/x-patch
Size: 11760 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201026/62a8c3e9/attachment-0001.bin>