This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by morehouse on Sep 14 2017, 12:33 PM.

Details

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.

Diff Detail

Event Timeline

morehouse created this revision.Sep 14 2017, 12:33 PM
morehouse edited the summary of this revision. (Show Details)Sep 14 2017, 12:35 PM
vitalybuka added inline comments.Sep 14 2017, 1:10 PM
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 requested changes to this revision.Sep 14 2017, 1:12 PM
vitalybuka added inline comments.
clang/test/Driver/fsanitize.c
175

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

This revision now requires changes to proceed.Sep 14 2017, 1:12 PM
morehouse added inline comments.Sep 14 2017, 1:50 PM
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 added inline comments.Sep 14 2017, 1:55 PM
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 edited edge metadata.

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 updated this revision to Diff 115312.Sep 14 2017, 4:02 PM
morehouse edited edge metadata.
morehouse edited the summary of this revision. (Show Details)

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.

What is the status of this patch?

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 planned changes to this revision.Nov 15 2017, 5:09 PM
  • 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.

PTAL. Patch has been updated.

eugenis accepted this revision.Jan 10 2018, 12:00 PM
vitalybuka accepted this revision.Jan 10 2018, 12:23 PM
This revision is now accepted and ready to land.Jan 10 2018, 12:23 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald TranscriptJan 10 2018, 12:29 PM