[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 ↗

(On Diff #115261)

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 ↗

(On Diff #115261)

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 ↗

(On Diff #115261)

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 ↗

(On Diff #115261)

Line 177 tests that default is ON.

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

13 ↗

(On Diff #115261)

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

[vitalybuka]

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

13 ↗

(On Diff #115261)

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.

test/msan/dtor-member.cc

 // RUN: %clangxx_msan %s -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 -O1 -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 -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
 // RUN: FileCheck %s --check-prefix=CHECK-NO-FLAG < %t.out
 
 #include <sanitizer/msan_interface.h>
 #include <assert.h>
 #include <stdio.h>

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>
 #include <stdio.h>
 #include <new>
 
 struct Simple {
   int x_;
   Simple() {
     x_ = 5;
   }
   ~Simple() {
     x_ += 1;
   }
 };
 
 int main() {
   unsigned long buf[1];
   assert(sizeof(Simple) <= sizeof(buf));
 
   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-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
 }