[PATCH] D101122: introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a subscriber: delcypher.
vitalybuka added a comment.

In D101122#2711962 <https://reviews.llvm.org/D101122#2711962>, @joerg wrote:

> There are two approaches that work for reviewing: starting from clang and going down or starting from compiler-rt and going up the layers. I'd prefer to do the latter as it means meaningful testing can be done easier. There are two natural steps here: clang flag+IR generation is one, llvm codegen and compiler-rt is the other. The clang step should include tests that ensure that proper IR is generated for the different flag combination (including documentation for the IR changes). The LLVM step should ensure that the different attributes (either function or module scope) are correctly lowered in the instrumentation pass and unit tests on the compiler-rt side to do functional testing. The unit testing might also be done as a third step if it goes the full way from C to binary code.

Today on chat I advised to start with clang, but now I agree with Joerg. More precisely you can go with following 4 patches:

1. LLVM Detailed IR tests of effect of this flag on transformation
2. compiler-rt (tests will have to uses -mllvm -<internal asan copt>)
3. clang
  - driver tests
  - basic IR tests to see that flag makes a difference of generated code. see llvm-project/clang/test/CodeGen/asan-*.cpp
4. compiler-rt:  replace -mllvm copt from patch2  with with clang flag from patch3.





================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:225
                                              ///< destructors are emitted.
+ENUM_CODEGENOPT(SanitizeAddressDetectStackUseAfterReturnMode,
+                llvm::AsanDetectStackUseAfterReturnMode, 2,
----------------
Mode->Kind to be consistent with the file


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:225-228
+ENUM_CODEGENOPT(SanitizeAddressDetectStackUseAfterReturnMode,
+                llvm::AsanDetectStackUseAfterReturnMode, 2,
+                llvm::AsanDetectStackUseAfterReturnMode::Runtime
+                ) ///< Set detection mode for stack-use-after-return.
----------------
vitalybuka wrote:
> Mode->Kind to be consistent with the file
I believe for consistency we need:
1. Var name (SanitizeAddressDetectStackUseAfterReturn) no Mode/Kind suffix
2. Enum type AsanDetectStackUseAfterReturnKind uses "kind"
3. Actual flag includes no "kind" -fsanitize-address-detect-stack-use-after-return=

BTW. @delcypher  -fsanitize-address-destructor-kind was not consistent with the rest of file, but I am not sure it's worth of fixing now.




================
Comment at: clang/include/clang/Driver/Options.td:1555
+      NormalizedValuesScope<"llvm::AsanDetectStackUseAfterReturnMode">,
+      NormalizedValues<["Always", "Runtime", "Never"]>,
+      MarshallingInfoEnum<CodeGenOpts<"SanitizeAddressDetectStackUseAfterReturnMode">, "Runtime">;
----------------
same order as enum


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h:29
+           ///< (ASAN_OPTIONS=detect_stack_use_after_return=1)
+  Never,   ///< Never detect stack use after return.
+  Invalid, ///< Not a valid detect mode.
----------------
Could you order them in acceding order by "strictness", this will make Never == 0, and the current default == 1.
Never = 0,  (maybe None for consistency?)
Runtime=1,
Always=2,
Invalid=<whatever>


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101122/new/

https://reviews.llvm.org/D101122