[PATCH] D31307: Enable builds of darwin lsan by default

[Francis Ricci via Phabricator via llvm-commits]

fjricci added inline comments.


================
Comment at: lib/asan/asan_flags.cc:64
     cf.CopyFrom(*common_flags());
-    cf.detect_leaks = CAN_SANITIZE_LEAKS;
+    cf.detect_leaks = CAN_SANITIZE_LEAKS && !SANITIZER_MAC;
     cf.external_symbolizer_path = GetEnv("ASAN_SYMBOLIZER_PATH");
----------------
kubamracek wrote:
> `asan_flags.cc` currently doesn't have any platform-specific code.  Could this be moved to some `_mac.cc` file?  Or perhaps into `sanitizer_flags.inc`?
> 
> If not, then this at least deserves a comment explaining that we're bringing up LSan and it doesn't fully work yet on Darwin.
I think the correct behavior here would be to account for the default value of `cf.detect_leaks`, as set in `sanitizer_flags.inc` when determining the default value for asan. However, this appears to be causing some problems with 32-bit lsan tests, where things should also be disabled by default. I'm looking into those test failures now.


================
Comment at: lib/asan/asan_flags.cc:64
     cf.CopyFrom(*common_flags());
-    cf.detect_leaks = CAN_SANITIZE_LEAKS;
+    cf.detect_leaks = CAN_SANITIZE_LEAKS && !SANITIZER_MAC;
     cf.external_symbolizer_path = GetEnv("ASAN_SYMBOLIZER_PATH");
----------------
fjricci wrote:
> kubamracek wrote:
> > `asan_flags.cc` currently doesn't have any platform-specific code.  Could this be moved to some `_mac.cc` file?  Or perhaps into `sanitizer_flags.inc`?
> > 
> > If not, then this at least deserves a comment explaining that we're bringing up LSan and it doesn't fully work yet on Darwin.
> I think the correct behavior here would be to account for the default value of `cf.detect_leaks`, as set in `sanitizer_flags.inc` when determining the default value for asan. However, this appears to be causing some problems with 32-bit lsan tests, where things should also be disabled by default. I'm looking into those test failures now.
I think the correct behavior here would be to account for the default value of cf.detect_leaks, as set in sanitizer_flags.inc when determining the default value for asan. However, this appears to be causing some problems with 32-bit lsan tests, where things should also be disabled by default. I'm looking into those test failures now.


https://reviews.llvm.org/D31307