introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.

[kda]

Summary

introduce flag -fsanitize-address-detect-stack-use-after-return-mode. No functional change.
for issue: https://github.com/google/sanitizers/issues/1394

[joerg]

This review doesn't make sense to me. Why add a flag that isn't affecting anything? Why add a flag that isn't even tested?

[kda]

I'm new here.
I was told to take small steps.
This will eventually effect compiler-rt and llvm.
I took the smallest possible step.

What I had previously: https://reviews.llvm.org/D99630
And was told it was too large.

[joerg]

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.

[vitalybuka]

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

clang/include/clang/Basic/CodeGenOptions.def

225

Mode->Kind to be consistent with the file

225–228

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.

clang/include/clang/Driver/Options.td

1555

same order as enum

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h

29

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>

[kda]

Revisions as requested.

[kda]

Thank you for delineating the steps.

Unfortunately, I don't know where to even start looking for the tests you mention in step 1. Are you looking for something like this: https://reviews.llvm.org/differential/changeset/?ref=2496958

For the 3 files I have here, which step are they in? (Step 1?)

[kda]

fixed up some more naming issues.

[delcypher]

clang/include/clang/Basic/CodeGenOptions.def

225–228

@vitalybuka It's not too late to fix this because the -fsanitize-address-destructor-kind= flag hasn't been adopted yet AFAIK.

Am I right in understanding that what I landed doesn't match points 1 and 3 above? If you can confirm that's what you'd like to be changed I can put up some patches to fix this.

[vitalybuka]

clang/include/clang/Basic/CodeGenOptions.def

225–228

Correct
I'd expect

ENUM_CODEGENOPT(SanitizeAddressDtor, llvm::AsanDtorKind, 2,
                llvm::AsanDtorKind::Global)

and

def sanitize_address_destructor_EQ
    : Joined<["-"], "fsanitize-address-destructor=">,
      MetaVarName<"<kind>">,
      Flags<[CC1Option]>,
      HelpText<"Set destructor type used in ASan instrumentation">,
      Group<f_clang_Group>,
      Values<"none,global">,
      NormalizedValuesScope<"llvm::AsanDtorKind">,
      NormalizedValues<["None", "Global"]>,
      MarshallingInfoEnum<CodeGenOpts<"SanitizeAddressDtor">, "Global">;

And one more thing, probably not very important, and matter of personal preferences.
Maybe we don't need MetaVarName<"<kind>"> for both of these flags.
Without MetaVarName documentation is going to be -fsanitize-address-destructor=<arg>
MetaVarName just lets use to replace <arg> with more common and specific <file>, <dir>.. etc.
To me <kind> is almost generic as <arg>, so why not to stick to default.

[delcypher]

clang/include/clang/Basic/CodeGenOptions.def

225–228

@vitalybuka I put up the following patches for review

https://reviews.llvm.org/D101490
https://reviews.llvm.org/D101491

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(SanitizeAddressGlobalsDeadStripping, 1, 0) ///< Enable linker dead stripping
                                                       ///< of globals in AddressSanitizer
 CODEGENOPT(SanitizeAddressUseOdrIndicator, 1, 0) ///< Enable ODR indicator globals
 CODEGENOPT(SanitizeMemoryTrackOrigins, 2, 0) ///< Enable tracking origins in
                                              ///< MemorySanitizer
 ENUM_CODEGENOPT(SanitizeAddressDtorKind, llvm::AsanDtorKind, 2,
                 llvm::AsanDtorKind::Global)  ///< Set how ASan global
                                              ///< destructors are emitted.
+ENUM_CODEGENOPT(SanitizeAddressDetectStackUseAfterReturn,
+                llvm::AsanDetectStackUseAfterReturnKind, 2,
+                llvm::AsanDetectStackUseAfterReturnKind::Runtime
+                ) ///< Set detection mode for stack-use-after-return.
 CODEGENOPT(SanitizeMemoryUseAfterDtor, 1, 0) ///< Enable use-after-delete detection
                                              ///< in MemorySanitizer
 CODEGENOPT(SanitizeCfiCrossDso, 1, 0) ///< Enable cross-dso support in CFI.
 CODEGENOPT(SanitizeMinimalRuntime, 1, 0) ///< Use "_minimal" sanitizer runtime for
                                          ///< diagnostics.
 CODEGENOPT(SanitizeCfiICallGeneralizePointers, 1, 0) ///< Generalize pointer types in
                                                      ///< CFI icall function signatures
 CODEGENOPT(SanitizeCfiCanonicalJumpTables, 1, 0) ///< Make jump table symbols canonical

clang/include/clang/Driver/Options.td

: Joined<["-"], "fsanitize-address-destructor-kind=">,
       MetaVarName<"<kind>">,
       Flags<[CC1Option]>,
       HelpText<"Set destructor type used in ASan instrumentation">,
       Group<f_clang_Group>,
       Values<"none,global">,
       NormalizedValuesScope<"llvm::AsanDtorKind">,
       NormalizedValues<["None", "Global"]>,
       MarshallingInfoEnum<CodeGenOpts<"SanitizeAddressDtorKind">, "Global">;
+def sanitize_address_detect_stack_use_after_return_EQ
+    : Joined<["-"], "fsanitize-address-detect-stack-use-after-return=">,
+      MetaVarName<"<kind>">,
+      Flags<[CC1Option]>,
+      HelpText<"Select the enabling method of detect-stack-use-after-return in AddressSanitizer">,
+      Group<f_clang_Group>,
+      Values<"never,runtime,always">,
+      NormalizedValuesScope<"llvm::AsanDetectStackUseAfterReturnKind">,
+      NormalizedValues<["Never", "Runtime", "Always"]>,
+      MarshallingInfoEnum<CodeGenOpts<"SanitizeAddressDetectStackUseAfterReturn">, "Runtime">;
 // Note: This flag was introduced when it was necessary to distinguish between
 //       ABI for correct codegen.  This is no longer needed, but the flag is
 //       not removed since targeting either ABI will behave the same.
 //       This way we cause no disturbance to existing scripts & code, and if we
 //       want to use this flag in the future we will cause no disturbance then
 //       either.
 def fsanitize_hwaddress_abi_EQ
     : Joined<["-"], "fsanitize-hwaddress-abi=">,

llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerOptions.h

 
 /// Types of ASan module destructors supported
 enum class AsanDtorKind {
   None,    ///< Do not emit any destructors for ASan
   Global,  ///< Append to llvm.global_dtors
   Invalid, ///< Not a valid destructor Kind.
   // TODO(dliew): Add more more kinds.
 };
+
+/// Kinds of ASan detect stack use after return
+enum class AsanDetectStackUseAfterReturnKind {
+  Never,   ///< Never detect stack use after return.
+  Runtime, ///< Detect stack use after return if runtime flag is enabled
+           ///< (ASAN_OPTIONS=detect_stack_use_after_return=1)
+  Always,  ///< Always detect stack use after return.
+  Invalid, ///< Not a valid detect mode.
+};
+
 } // namespace llvm
+
 #endif