Enable 64-bit Darwin LeakSanitizer by default on AddressSanitizer builds

[fjricci]

Summary

LSan on Darwin was originally disabled by default for a couple reasons:

1. To give the opportunity to test thoroughly
2. The performance wasn't great

In terms of the first item, I've been running LSan on a large internal project
(10s of thousands of files of c++ and objc) for about a month and things are working well,
the full test suite for the project runs and passes with LSan enabled on Darwin.

The performance issues have been largely mitigated, and the slowdown over stock ASan
is similar to Linux (10-15% on the tests I've looked at).

[kubamracek]

Great work on LSan, thank you!

Please wait on enabling this on-by-default until we perform some more testing on our side.

Specifically, we need to make sure we don't get false positives. Have you tried e.g. running this on apps and programs written in Swift? Does bootstrap'd LLVM/Clang report no leaks?

[fjricci]

Please wait on enabling this on-by-default until we perform some more testing on our side.

You will probably want to wait until D35432 goes in, as that has the potential to introduce false positives (although I didn't see any in my testing). This patch is blocked on that anyway.

Have you tried e.g. running this on apps and programs written in Swift?

I don't have access to much in the way of Swift code, but I can try adding it to the test suite and see how that goes.

Does bootstrap'd LLVM/Clang report no leaks?

Didn't think to run the clang test suite, working on that now.

[fjricci]

I ran bootstrapped check-clang with -fsanitize=address and this patch, all tests passed. (Verified that it was running LSan as well by making sure tests failed with a CHECK(0) in DoLeakCheck()).

Unfortunately, I don't think it's currently possible to run this on swift. Darwin LSan requires clang-5, and the master-next branch does not currently build due to some KPs with debug info.

[fjricci]

I spoke too soon, looks like check-clang is clean but check-llvm has a few leaks. I'll look into whether those seem like false positives.

[kubamracek]

In D35443#812065, @fjricci wrote:

I ran bootstrapped check-clang with -fsanitize=address and this patch, all tests passed. (Verified that it was running LSan as well by making sure tests failed with a CHECK(0) in DoLeakCheck()).

Unfortunately, I don't think it's currently possible to run this on swift. Darwin LSan requires clang-5, and the master-next branch does not currently build due to some KPs with debug info.

I see. But mostly it's important that LSan works against Swift programs/apps rather than the compiler. How about taking some existing swift binaries or building some swift app with Xcode 8 or Xcode 9 (in beta now) and just using LSan against an already built binary. Since LSan is a runtime-only tool, that should work, right?

[fjricci]

re check-llvm, turns out it was a KP with libedit, re-running with that disabled using LLVM_USE_SANITIZER instead of manual cflags

I see. But mostly it's important that LSan works against Swift programs/apps rather than the compiler. How about taking some existing swift binaries or building some swift app with Xcode 8 or Xcode 9 (in beta now) and just using LSan against an already built binary. Since LSan is a runtime-only tool, that should work, right?

LSan is runtime-only, but I believe the linker needs to know about it (otherwise the binary wouldn't know to load the lsan dso for the interceptors etc). -fsanitize=leak is a no-op at compile time, but it's used at link time afaik.

[kubamracek]

LSan is runtime-only, but I believe the linker needs to know about it (otherwise the binary wouldn't know to load the lsan dso for the interceptors etc). -fsanitize=leak is a no-op at compile time, but it's used at link time afaik.

DYLD_INSERT_LIBRARIES=<path_to_lsan_library> ./an-already-built-binary

should do the trick.

[fjricci]

DYLD_INSERT_LIBRARIES=<path_to_lsan_library> ./an-already-built-binary
should do the trick.

Awesome thanks

[fjricci]

Still working on swift, but looks like bootstrapped llvm-tblgen has a few leaks. They don't show up on Linux, but they also don't really look like false positives:

Direct leak of 61440 byte(s) in 256 object(s) allocated from:

#0 0x1030e30eb in wrap__Znwm (/Users/fjricci/Source/llvm/build/lib/clang/5.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x600eb)
#1 0x1029b9f56 in (anonymous namespace)::TupleExpander::expand(llvm::SetTheory&, llvm::Record*, llvm::SmallSetVector<llvm::Record*, 16u>&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100161f56)
#2 0x102cb0f3e in llvm::SetTheory::expand(llvm::Record*) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100458f3e)
#3 0x102990ad4 in llvm::CodeGenRegBank::CodeGenRegBank(llvm::RecordKeeper&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100138ad4)
#4 0x102a0f788 in llvm::CodeGenTarget::getRegBank() const (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x1001b7788)
#5 0x1028b7f82 in llvm::EmitAsmWriter(llvm::RecordKeeper&, llvm::raw_ostream&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x10005ff82)
#6 0x102bdca7a in (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, llvm::RecordKeeper&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100384a7a)
#7 0x102c7f1d3 in llvm::TableGenMain(char*, bool (*)(llvm::raw_ostream&, llvm::RecordKeeper&)) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x1004271d3)
#8 0x102bdc390 in main (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100384390)
#9 0x7fffaf267234 in start (/usr/lib/system/libdyld.dylib:x86_64+0x5234)

Indirect leak of 80640 byte(s) in 224 object(s) allocated from:

#0 0x1030d7f30 in wrap_realloc (/Users/fjricci/Source/llvm/build/lib/clang/5.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib:x86_64+0x54f30)
#1 0x102c2e5c9 in llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x1003d65c9)
#2 0x1029bae01 in (anonymous namespace)::TupleExpander::expand(llvm::SetTheory&, llvm::Record*, llvm::SmallSetVector<llvm::Record*, 16u>&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100162e01)
#3 0x102cb0f3e in llvm::SetTheory::expand(llvm::Record*) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100458f3e)
#4 0x102990ad4 in llvm::CodeGenRegBank::CodeGenRegBank(llvm::RecordKeeper&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100138ad4)
#5 0x102a0f788 in llvm::CodeGenTarget::getRegBank() const (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x1001b7788)
#6 0x1028b7f82 in llvm::EmitAsmWriter(llvm::RecordKeeper&, llvm::raw_ostream&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x10005ff82)
#7 0x102bdca7a in (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, llvm::RecordKeeper&) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100384a7a)
#8 0x102c7f1d3 in llvm::TableGenMain(char*, bool (*)(llvm::raw_ostream&, llvm::RecordKeeper&)) (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x1004271d3)
#9 0x102bdc390 in main (/Users/fjricci/Source/llvm/bootstrap/bin/llvm-tblgen:x86_64+0x100384390)
#10 0x7fffaf267234 in start (/usr/lib/system/libdyld.dylib:x86_64+0x5234)

[fjricci]

I'm running the swift stdlib test suite with LSan, and it complains about the dyld insertion:

dyld: warning: could not load inserted library '/Users/fjricci/Source/llvm/build/lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib' into library validated process because no suitable image found.  Did find:
	/Users/fjricci/Source/llvm/build/lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib: code signature in (/Users/fjricci/Source/llvm/build/lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib) not valid for use in process using Library Validation: mapped file has no Team ID and is not a platform binary (signed with custom identity or adhoc?)

In addition to those warnings, it also reports a lot of leaks. For example:
Direct leak of 1024 byte(s) in 1 object(s) allocated from:

#0 0x10f81313e in wrap_malloc sanitizer_malloc_mac.inc:137
#1 0x1108bc788 in swift_slowAlloc Heap.cpp:28
#2 0x10f7ce9b0 in closure #1 in closure #8 in  (a.out_swift4:x86_64+0x1000039b0)
#3 0x10f7df26b in partial apply for closure #1 in closure #8 in  (a.out_swift4:x86_64+0x10001426b)
#4 0x10f7cead5 in thunk for @callee_owned () -> (@error @owned Error) (a.out_swift4:x86_64+0x100003ad5)
#5 0x10f7df312 in partial apply for thunk for @callee_owned () -> (@error @owned Error) (a.out_swift4:x86_64+0x100014312)
#6 0x11152e6f7 in autoreleasepool<A>(invoking:) ObjectiveC.swift:177
#7 0x10f7ce89d in closure #8 in  (a.out_swift4:x86_64+0x10000389d)
#8 0x1115a3898 in specialized TestSuite._runTest(name:parameter:) StdlibUnittest.swift:1708
#9 0x11154d653 in _childProcess() <unknown>
#10 0x11155263f in runAllTests() StdlibUnittest.swift:1587
#11 0x10f7cbff8 in $defer #1 () in  (a.out_swift4:x86_64+0x100000ff8)
#12 0x10f7cd909 in main (a.out_swift4:x86_64+0x100002909)
#13 0x7fffaf267234 in start (libdyld.dylib:x86_64+0x5234)

Direct leak of 44 byte(s) in 1 object(s) allocated from:

#0 0x10f81313e in wrap_malloc sanitizer_malloc_mac.inc:137
#1 0x7fffaf29b873 in _Block_copy (libsystem_blocks.dylib:x86_64+0x873)
#2 0x7fffaf231b75 in _dispatch_Block_copy (libdispatch.dylib:x86_64+0x1b75)
#3 0x7fffaf239e44 in dispatch_async (libdispatch.dylib:x86_64+0x9e44)
#4 0x10f812727 in wrap_dispatch_async lsan_mac.cc:160
#5 0x111082028 in _swift_dispatch_syncTm DispatchOverlayShims.h:129
#6 0x11107554b in DispatchQueue.async(execute:) Queue.swift:184
#7 0x10f7ce5dd in closure #6 in  (a.out_swift4:x86_64+0x1000035dd)
#8 0x1115a3898 in specialized TestSuite._runTest(name:parameter:) StdlibUnittest.swift:1708
#9 0x11154d653 in _childProcess() <unknown>
#10 0x11155263f in runAllTests() StdlibUnittest.swift:1587
#11 0x10f7cbff8 in $defer #1 () in  (a.out_swift4:x86_64+0x100000ff8)
#12 0x10f7cd909 in main (a.out_swift4:x86_64+0x100002909)
#13 0x7fffaf267234 in start (libdyld.dylib:x86_64+0x5234)

Assuming that these are false positives and not real leaks, and that we care about swift support, we should hold off on this patch.

[fjricci]

Hi @kubamracek - turns out that the llvm-tblgen issues were caused by DCE leading to a broken weak hook, which I fixed here: https://reviews.llvm.org/D37636.

Bootstrapped check-clang reports only 2 test failures with this patch. One is a known leak in OMP arg parsing, which I've tried fixing, but haven't had much luck on since I'm not familiar with the codebase: https://reviews.llvm.org/D34784. Presumably that leak doesn't show up on buildbots currently because the test is disabled on Linux. The other failing test also doesn't run on linux, and is a forced crash test (crash-vfs-ivfsoverlay.m). Perhaps that one is failing because LSan changes exit behavior? LSan doesn't actually report a false positive leak there, it just omits some of the crash dump printout.

What's the preferred way to handle these failures? I think the current state demonstrates that LeakSanitizer itself works well. How important is swift support? I haven't had much time to look into that, since hasn't been a big priority for me.

[kubamracek]

Thanks for working on this!

LSan doesn't actually report a false positive leak there, it just omits some of the crash dump printout.
What's the preferred way to handle these failures?

Why does it omit some of the output? That sounds weird. In any case, for clang test failures, if we think the value of running LSan is high enough already, we can just disable the two failing tests under ASan/LSan. I don't have a problem with that.

What is controversial is turning on detect_leaks=1 for everyone who currently uses ASan. Clang/LLVM is not a representative codebase and having no false positives on Clang/LLVM doesn't mean we don't have problems elsewhere. We (Apple) have many users of ASan on various codebases written in C, C++, Objective-C and also Swift. I mentioned Swift in particular because the language runtime plays a lot of tricks of pointer bitmasking and reusing pointer bits to store data — these things can easily cause false positives. Maybe this concern is

Secondly, not everyone cares about leaks. There are programs that are short-lived and their authors simply do not see leaks as a serious problem.

The way I see it, I'd prefer users to opt in to use LSan. If a project wants to detect leaks, it can just set ASAN_OPTIONS=detect_leaks=1. Do you have a strong reason why detect_leaks=1 should be the default? (Besides parity with Linux.)

Setting detect_leaks=1 in the ASan test suite is, of course, a good thing to do in any case.

[fjricci]

Why does it omit some of the output?

My assumption was that the crash handlers may run inside atexit() or something similar, which would conflict with LSan, but I didn't dig too much into the way clang's crash handlers work.

In any case, for clang test failures, if we think the value of running LSan is high enough already, we can just disable the two failing tests under ASan/LSan. I don't have a problem with that.

Makes sense to me

The way I see it, I'd prefer users to opt in to use LSan. If a project wants to detect leaks, it can just set ASAN_OPTIONS=detect_leaks=1. Do you have a strong reason why detect_leaks=1 should be the default? (Besides parity with Linux.)

I think my main interest is related to getting the info out about it, I've spoken to a couple ASan users from other teams/companies who were interested in darwin+LSan but weren't aware it was now supported. Perhaps a better way to accomplish this would be just updating the documentation to reflect that LSan exists on Darwin now (probably with some sort of beta designation)? I assume the lightning talk would also help with visibility.

Setting detect_leaks=1 in the ASan test suite is, of course, a good thing to do in any case.

It already is the default in the asan suite, but could be good to turn it on for the clang suite as well.

[fjricci]

Abandoning in favor of other options.

lib/sanitizer_common/sanitizer_flags.inc

COMMON_FLAG(
     "append executable name to logs (as in \"log_path.exe_name.pid\").")
 COMMON_FLAG(
     bool, log_to_syslog, SANITIZER_ANDROID || SANITIZER_MAC,
     "Write all sanitizer output to syslog in addition to other means of "
     "logging.")
 COMMON_FLAG(
     int, verbosity, 0,
     "Verbosity level (0 - silent, 1 - a bit of output, 2+ - more output).")
-COMMON_FLAG(bool, detect_leaks, !SANITIZER_MAC, "Enable memory leak detection.")
+COMMON_FLAG(bool, detect_leaks, !(SANITIZER_MAC && (SANITIZER_WORDSIZE != 64)),
+            "Enable memory leak detection.")
 COMMON_FLAG(
     bool, leak_check_at_exit, true,
     "Invoke leak checking in an atexit handler. Has no effect if "
     "detect_leaks=false, or if __lsan_do_leak_check() is called before the "
     "handler has a chance to run.")
 COMMON_FLAG(bool, allocator_may_return_null, false,
             "If false, the allocator will crash instead of returning 0 on "
             "out-of-memory.")

test/asan/lit.cfg

 # Platform-specific default ASAN_OPTIONS for lit tests.
 default_asan_opts = ''
 if config.host_os == 'Darwin':
   # On Darwin, we default to `abort_on_error=1`, which would make tests run
   # much slower. Let's override this and run lit tests with 'abort_on_error=0'.
   # Also, make sure we do not overwhelm the syslog while testing.
   default_asan_opts = 'abort_on_error=0'
   default_asan_opts += ':log_to_syslog=0'
-
-  # On Darwin, leak checking is not enabled by default. Enable for x86_64
-  # tests to prevent regressions
-  if config.target_arch == 'x86_64':
-    default_asan_opts += ':detect_leaks=1'
 elif config.android:
   # The same as on Darwin, we default to "abort_on_error=1" which slows down
   # testing. Also, all existing tests are using "not" instead of "not --crash"
   # which does not work for abort()-terminated programs.
   default_asan_opts = 'abort_on_error=0'
 
 if default_asan_opts:
   config.environment['ASAN_OPTIONS'] = default_asan_opts