[clang][msan] Turn on -fsanitize-memory-param-retval by default

[aeubanks]

Summary

This eagerly reports use of undef values when passed to noundef
parameters or returned from noundef functions.

This also decreases binary sizes under msan.

To go back to the previous behavior, pass -fno-sanitize-memory-param-retval.

[aeubanks]

hmm, looks like this breaks a bunch of msan tests with -DLLVM_ENABLE_RUNTIMES=compiler-rt and ninja && ninja check-runtimes

MemorySanitizer-X86_64 :: Linux/swapcontext_annotation_reset.cpp
MemorySanitizer-X86_64 :: bsearch.cpp
MemorySanitizer-X86_64 :: chained_origin.cpp
MemorySanitizer-X86_64 :: chained_origin_empty_stack.cpp
MemorySanitizer-X86_64 :: chained_origin_memcpy.cpp
MemorySanitizer-X86_64 :: chained_origin_memmove.cpp
MemorySanitizer-X86_64 :: cxa_atexit.cpp
MemorySanitizer-X86_64 :: insertvalue_origin.cpp
MemorySanitizer-X86_64 :: no_sanitize_memory_prop.cpp
MemorySanitizer-X86_64 :: noundef_analysis.cpp
MemorySanitizer-X86_64 :: param_tls_limit.cpp
MemorySanitizer-X86_64 :: qsort.cpp
MemorySanitizer-X86_64 :: signal_stress_test.cpp
MemorySanitizer-X86_64 :: unpoison_param.cpp
MemorySanitizer-X86_64 :: vector_cvt.cpp
SanitizerCommon-msan-x86_64-Linux :: get_module_and_offset_for_pc.cpp
UBSan-MemorySanitizer-x86_64 :: TestCases/Misc/monitor.cpp
UBSan-MemorySanitizer-x86_64 :: TestCases/TypeCheck/vptr.cpp

[aeubanks]

before looking into those, is it ok to proceed with this?

[vitalybuka]

LGTM. I was thinking about this recently as well.
Main inconvenience is users like Chromium. They have precompiled libraries. They will need to use -fno-sanitize-memory-param-retval, if recompiling is not an option.

[aeubanks]

do you want the tests to all properly work with -fsanitize-memory-param-retval, or just pin them to -fno-sanitize-memory-param-retval if they're failing?

[vitalybuka]

In D134669#3816519, @aeubanks wrote:

do you want the tests to all properly work with -fsanitize-memory-param-retval, or just pin them to -fno-sanitize-memory-param-retval if they're failing?

I have some draft patch for D134683.

[vitalybuka]

clang/include/clang/Driver/Options.td

1767

you need to update SanitizerArgs::MsanParamRetval

SanitizerArgs.cpp:1187

if (!MsanParamRetval)
    CmdArgs.push_back("-fno-sanitize-memory-param-retval");

without that option.td is no-op

Then D134683 works with and without your patch

[vitalybuka]

Update MsanParamRetval

[aeubanks]

add release notes

[vitalybuka]

diff --git a/clang/test/Driver/fsanitize-memory-param-retval.c b/clang/test/Driver/fsanitize-memory-param-retval.c
index d82d20812186..79ade32178b6 100644
--- a/clang/test/Driver/fsanitize-memory-param-retval.c
+++ b/clang/test/Driver/fsanitize-memory-param-retval.c
@@ -1,14 +1,14 @@
-// RUN: %clang -target i386-gnu-linux %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
-// RUN: %clang -target x86_64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
-// RUN: %clang -target aarch64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
-// RUN: %clang -target riscv32-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
-// RUN: %clang -target riscv64-linux-gnu %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
-// RUN: %clang -target x86_64-linux-gnu %s -fsanitize=kernel-memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target i386-gnu-linux %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-gnu %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target aarch64-linux-gnu %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32-linux-gnu %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64-linux-gnu %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target x86_64-linux-gnu %s -fsanitize=kernel-memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck %s
 
-// CHECK: "-fsanitize-memory-param-retval"
+// CHECK: "-fno-sanitize-memory-param-retval"
 
-// RUN: %clang -target aarch64-linux-gnu -fsyntax-only %s -fsanitize=memory -fsanitize-memory-param-retval -c -### 2>&1 | FileCheck --check-prefix=11 %s
-// 11: "-fsanitize-memory-param-retval"
+// RUN: %clang -target aarch64-linux-gnu -fsyntax-only %s -fsanitize=memory -fno-sanitize-memory-param-retval -c -### 2>&1 | FileCheck --check-prefix=11 %s
+// 11: "-fno-sanitize-memory-param-retval"
 
-// RUN: not %clang -target x86_64-linux-gnu -fsyntax-only %s -fsanitize=memory -fsanitize-memory-param-retval=1 2>&1 | FileCheck --check-prefix=EXCESS %s
-// EXCESS: error: unknown argument: '-fsanitize-memory-param-retval=
+// RUN: not %clang -target x86_64-linux-gnu -fsyntax-only %s -fsanitize=memory -fno-sanitize-memory-param-retval=1 2>&1 | FileCheck --check-prefix=EXCESS %s
+// EXCESS: error: unknown argument: '-fno-sanitize-memory-param-retval=

[aeubanks]

update test

[aeubanks]

rebase (to run through presubmits with test fixes)

[xbolva00]

clang/docs/ReleaseNotes.rst

452 ↗

(On Diff #463322)

Probably I would mention that there is -fno-sanitize-memory-param-retval to disable it.

So one can use “-fsanitize=memory -fno-sanitize-memory-param-retval“ to unblock failed builds.

[aeubanks]

mention -fno-sanitize-memory-param-retval in release notes

clang/include/clang/Driver/Options.td

def sanitize_address_destructor_EQ
     : Joined<["-"], "fsanitize-address-destructor=">,
       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">;
 defm sanitize_memory_param_retval
     : BoolFOption<"sanitize-memory-param-retval",
         CodeGenOpts<"SanitizeMemoryParamRetval">,
-        DefaultFalse,
+        DefaultTrue,
         PosFlag<SetTrue, [CC1Option], "Enable">, NegFlag<SetFalse, [], "Disable">,
         BothFlags<[], " detection of uninitialized parameters and return values">>;
 //// 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.

clang/include/clang/Driver/SanitizerArgs.h

class SanitizerArgs {
   std::vector<std::string> UserIgnorelistFiles;
   std::vector<std::string> SystemIgnorelistFiles;
   std::vector<std::string> CoverageAllowlistFiles;
   std::vector<std::string> CoverageIgnorelistFiles;
   int CoverageFeatures = 0;
   int BinaryMetadataFeatures = 0;
   int MsanTrackOrigins = 0;
   bool MsanUseAfterDtor = true;
-  bool MsanParamRetval = false;
+  bool MsanParamRetval = true;
   bool CfiCrossDso = false;
   bool CfiICallGeneralizePointers = false;
   bool CfiCanonicalJumpTables = false;
   int AsanFieldPadding = 0;
   bool SharedRuntime = false;
   bool AsanUseAfterScope = true;
   bool AsanPoisonCustomArrayCookie = false;
   bool AsanGlobalsDeadStripping = false;

clang/lib/Driver/SanitizerArgs.cpp

void SanitizerArgs::addArgs(const ToolChain &TC, const llvm::opt::ArgList &Args,
 
   if (MsanTrackOrigins)
     CmdArgs.push_back(Args.MakeArgString("-fsanitize-memory-track-origins=" +
                                          Twine(MsanTrackOrigins)));
 
   if (MsanUseAfterDtor)
     CmdArgs.push_back("-fsanitize-memory-use-after-dtor");
 
-  if (MsanParamRetval)
-    CmdArgs.push_back("-fsanitize-memory-param-retval");
+  if (!MsanParamRetval)
+    CmdArgs.push_back("-fno-sanitize-memory-param-retval");
 
   // FIXME: Pass these parameters as function attributes, not as -llvm flags.
   if (!TsanMemoryAccess) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-tsan-instrument-memory-accesses=0");
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-tsan-instrument-memintrinsics=0");
   }

clang/test/CodeGen/kmsan-param-retval.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -no-enable-noundef-analysis -o - %s | \
 // RUN:     FileCheck %s --check-prefix=CLEAN
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -o - %s | \
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -fno-sanitize-memory-param-retval -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,NOUNDEF_ONLY
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -mllvm -msan-eager-checks -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,EAGER
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -no-enable-noundef-analysis -fsanitize-memory-param-retval -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=CLEAN
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -fsanitize-memory-param-retval -o - %s | \
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -O2 -fsanitize=kernel-memory -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,EAGER
 
 void foo();
 
 void bar(int x) {
   if (x)
     foo();
 }

clang/test/CodeGen/msan-param-retval.c

 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -no-enable-noundef-analysis -o - %s | \
 // RUN:     FileCheck %s --check-prefix=CLEAN
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -o - %s | \
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -fno-sanitize-memory-param-retval -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,NOUNDEF_ONLY
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -mllvm -msan-eager-checks -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,EAGER
 // RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -no-enable-noundef-analysis -fsanitize-memory-param-retval -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=CLEAN
-// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -fsanitize-memory-param-retval -o - %s | \
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -S -emit-llvm -fsanitize=memory -o - %s | \
 // RUN:     FileCheck %s --check-prefixes=NOUNDEF,EAGER
 
 void bar(int x) {
 }
 
 // CLEAN:   define dso_local void @bar(i32 %x) #0 {
 // NOUNDEF: define dso_local void @bar(i32 noundef %x) #0 {
 // CLEAN:        @__msan_param_tls
 // NOUNDEF_ONLY: @__msan_param_tls
 // EAGER-NOT:    @__msan_param_tls
 No newline at end of file