[MSan] Enable use-after-dtor instrumentation by default.

[morehouse]

Summary

Enable the compile-time flag -fsanitize-memory-use-after-dtor by
default. Note that the run-time option MSAN_OPTIONS=poison_in_dtor=1
still needs to be enabled for destructors to be poisoned.

[vitalybuka]

clang/include/clang/Driver/Options.td

854 ↗

(On Diff #115261)

I'd recommend to split patch in two:

1. Add no-sanitize with tests
2. Flip default

clang/lib/Driver/SanitizerArgs.cpp

492

Can you use parsing similar to AsanUseAfterScope at line 645

compiler-rt/test/msan/dtor-bit-fields.cc

3 ↗

(On Diff #115261)

Please keep -fsanitize-memory-use-after-dtor in dtor* tests.
We test the check itself here, not default value. So we don't flip it every time default switched.

compiler-rt/test/msan/use-after-dtor.cc

13

Probably we need one test which check that we stop detecting bugs with
-fno-sanitize-memory-track-origins. Maybe this one or in separate file.

[vitalybuka]

clang/test/Driver/fsanitize.c

175

Somewhere we need to have test that default is ON or OFF.
Maybe here?

[morehouse]

clang/include/clang/Driver/Options.td

854 ↗

(On Diff #115261)

Can do.

clang/test/Driver/fsanitize.c

175

Line 177 tests that default is ON.

compiler-rt/test/msan/use-after-dtor.cc

13

Do you mean -fno-sanitize-memory-use-after-dtor? See dtor-member.cc test.

[vitalybuka]

compiler-rt/test/msan/use-after-dtor.cc

13

Right.
use-after-dtor.cc seems more general than dtor-member.cc. So maybe having it here as well would be nice.

[eugenis]

Looking at __sanitizer_dtor_callback implementation, this change will add a (fast) stack unwind in every destructor. In extreme cases (like a tight loop doing string operations) it could be bad for performance. I've seen ~15% AFAIR.

[morehouse]

• Move the new flag to https://reviews.llvm.org/D37867
• Address Vitaly's comments.

[morehouse]

In D37860#871368, @eugenis wrote:

Looking at __sanitizer_dtor_callback implementation, this change will add a (fast) stack unwind in every destructor. In extreme cases (like a tight loop doing string operations) it could be bad for performance. I've seen ~15% AFAIR.

I haven't looked at performance, but I'll take a look at your internal bug. Until that is resolved, this can be on hold.

[vitalybuka]

What is the status of this patch?

[morehouse]

Patch is out-of-date. But the flag has been enabled internally for over a month with no issues. I'll update this patch soon, so we can flip the default here.

[morehouse]

• Enable use-after-dtor instrumentation by default.
• Make sanitize-no-dtor-callback.cpp test fail with UAD instrumentation.
• Update test cases to reflect new default.

[morehouse]

PTAL. Patch has been updated.

clang/lib/Driver/SanitizerArgs.cpp

     MsanUseAfterDtor =
         Args.hasFlag(options::OPT_fsanitize_memory_use_after_dtor,
                      options::OPT_fno_sanitize_memory_use_after_dtor,
-                     false);
+                     true);
     NeedPIE |= !(TC.getTriple().isOSLinux() &&
                  TC.getTriple().getArch() == llvm::Triple::x86_64);
   }

clang/test/CodeGenCXX/sanitize-no-dtor-callback.cpp

-// Test without the flag -fsanitize-memory-use-after-dtor, to ensure that
+// Test with the flag -fno-sanitize-memory-use-after-dtor, to ensure that
 // instrumentation is not erroneously inserted
-// RUN: %clang_cc1 -fsanitize=memory -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=memory -fno-sanitize-memory-use-after-dtor -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
 
 struct Simple {
+  int x;
   ~Simple() {}
 };
 Simple s;

 // CHECK-NOT: call void @__sanitizer_dtor_callback
 
 struct Inlined {
+  int y;
   inline ~Inlined() {}
 };
 Inlined i;

clang/test/Driver/fsanitize.c

 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=memory -fsanitize-memory-use-after-dtor %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=memory -fno-sanitize-memory-use-after-dtor -fsanitize-memory-use-after-dtor %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR
 // CHECK-USE-AFTER-DTOR: -cc1{{.*}}-fsanitize-memory-use-after-dtor
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=memory -fno-sanitize-memory-use-after-dtor %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR-OFF
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=memory -fsanitize-memory-use-after-dtor -fno-sanitize-memory-use-after-dtor %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR-OFF
-// RUN: %clang -target x86_64-linux-gnu -fsanitize=memory %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-USE-AFTER-DTOR-OFF
 // CHECK-USE-AFTER-DTOR-OFF-NOT: -cc1{{.*}}memory-use-after-dtor
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-address-field-padding=0 %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-FIELD-PADDING-0

compiler-rt/test/msan/dtor-member.cc

 // RUN: %clangxx_msan %s -O2 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1  %run %t >%t.out 2>&1
 // RUN: FileCheck %s < %t.out
 
-// RUN: %clangxx_msan %s -fsanitize=memory -o %t && MSAN_OPTIONS=poison_in_dtor=1  %run %t >%t.out 2>&1
+// RUN: %clangxx_msan %s -fsanitize=memory -fno-sanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1  %run %t >%t.out 2>&1
 // RUN: FileCheck %s --check-prefix=CHECK-NO-FLAG < %t.out
 
 // RUN: %clangxx_msan -fsanitize=memory -fsanitize-memory-use-after-dtor %s -o %t && MSAN_OPTIONS=poison_in_dtor=0 %run %t >%t.out 2>&1

compiler-rt/test/msan/use-after-dtor.cc

 // RUN: %clangxx_msan %s -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 not %run %t >%t.out 2>&1
-// RUN: FileCheck %s < %t.out
+// RUN: FileCheck %s --check-prefix=CHECK-UAD < %t.out
 
 // RUN: %clangxx_msan %s -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 not %run %t >%t.out 2>&1
-// RUN: FileCheck %s < %t.out
+// RUN: FileCheck %s --check-prefix=CHECK-UAD < %t.out
 
 // RUN: %clangxx_msan %s -O2 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 not %run %t >%t.out 2>&1
-// RUN: FileCheck %s < %t.out
+// RUN: FileCheck %s --check-prefix=CHECK-UAD < %t.out
 
 // RUN: %clangxx_msan %s -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -fsanitize-memory-track-origins -o %t && MSAN_OPTIONS=poison_in_dtor=1 not %run %t >%t.out 2>&1
-// RUN: FileCheck %s --check-prefix=CHECK-ORIGINS < %t.out
+// RUN: FileCheck %s --check-prefixes=CHECK-UAD,CHECK-ORIGINS < %t.out
+
+// RUN: %clangxx_msan %s -fno-sanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 not %run %t >%t.out 2>&1
+// RUN: FileCheck %s --check-prefix=CHECK-UAD-OFF < %t.out
 
 #include <sanitizer/msan_interface.h>
 #include <assert.h>

   Simple *s = new(&buf) Simple();
   s->~Simple();
 
+  fprintf(stderr, "\n");  // Need output to parse for CHECK-UAD-OFF case
   return s->x_;
 
-  // CHECK: WARNING: MemorySanitizer: use-of-uninitialized-value
-  // CHECK: {{#0 0x.* in main.*use-after-dtor.cc:}}[[@LINE-3]]
+  // CHECK-UAD: WARNING: MemorySanitizer: use-of-uninitialized-value
+  // CHECK-UAD: {{#0 0x.* in main.*use-after-dtor.cc:}}[[@LINE-3]]
+  // CHECK-UAD-OFF-NOT: WARNING: MemorySanitizer: use-of-uninitialized-value
 
   // CHECK-ORIGINS: Memory was marked as uninitialized
   // CHECK-ORIGINS: {{#0 0x.* in __sanitizer_dtor_callback}}
   // CHECK-ORIGINS: {{#1 0x.* in Simple::~Simple}}
 
-  // CHECK: SUMMARY: MemorySanitizer: use-of-uninitialized-value {{.*main}}
+  // CHECK-UAD: SUMMARY: MemorySanitizer: use-of-uninitialized-value {{.*main}}
+  // CHECK-UAD-OFF-NOT: SUMMARY: MemorySanitizer: use-of-uninitialized-value
 }