[lsan][Darwin] Scan libdispatch and Foundation memory regions

[lgrey]

Summary

libdispatch uses its own heap (_dispatch_main_heap) for some allocations, including the dispatch_continuation_t that holds a dispatch source's event handler.
Objective-C block trampolines (creating methods at runtime with a block as the implementations) use the VM_MEMORY_FOUNDATION region (see https://github.com/apple-oss-distributions/objc4/blob/8701d5672d3fd3cd817aeb84db1077aafe1a1604/runtime/objc-block-trampolines.mm#L371).

This change scans both regions to fix false positives. See tests for details; unfortunately I was unable to reduce the trampoline example with imp_implementationWithBlock on a new class, so I'm resorting to something close to the bug as seen in the wild.

[fjricci]

Hey sorry, it’s been several years since I’ve done anything LLVM-related, I don’t think I’m still the right reviewer for this code.

I actually don’t even write C++ anymore.

[yln]

Hi Leonard!

Does this fix the check-lsan and check-asan targets (currently many tests are failing there on macOS Beta 13)?

If this fixes a specific problem not already exposed by existing tests, then can we try to add a test for it?
If this fixes existing tests, can you mention in the commit messages which ones they are?

Thanks!

[yln]

This comment was removed by yln.

[lgrey]

In D129385#3639506, @yln wrote:

Does this fix the check-lsan and check-asan targets (currently many tests are failing there on macOS Beta 13)?
Thanks!

To be perfectly honest, I'm not sure since the objc runtime issues are masking everything else! I'm going to send a separate change to suppress those and get back to you :) (For context, I'm working on enabling LSAN for Mac Chromium, so there's probably a few more of these in the pipeline).

Are there any public builders running these suites on 13? My 13 machine doesn't have a dev environment so I'd be curious to see if there's anything happening beyond what I'm seeing on 11/12.

[lgrey]

In D129385#3639640, @lgrey wrote:

To be perfectly honest, I'm not sure since the objc runtime issues are masking everything else! I'm going to send a separate change to suppress those and get back to you :)

On second thought: What I'm realizing is that suppressing ObjC runtime errors is not great, since they're so common. The stack still needs to be symbolized to do the suppression, so everything is very slow.

Instead of suppressing, WDYT about just ignoring the relevant regions? Details on why this happens here: https://docs.google.com/document/d/11h3N06tN-_n7nbwVK8qTRnWz3paICI_HlrVM8BvmZ9s

[yln]

In D129385#3639640, @lgrey wrote:

I'm working on enabling LSAN for Mac Chromium

Very cool and good to know! :)

Are there any public builders running these suites on 13? My 13 machine doesn't have a dev environment so I'd be curious to see if there's anything happening beyond what I'm seeing on 11/12.

Apple doesn't support LSan because we ship the leaks tool in our toolchain already that serves this purpose. I don't think we are running check-lsan anywhere.
However, I think that this mostly worked on macOS until recently (before macOS 13?) so I initially thought that your are fixing that problem.

Instead of suppressing, WDYT about just ignoring the relevant regions? Details on why this happens here: https://docs.google.com/document/d/11h3N06tN-_n7nbwVK8qTRnWz3paICI_HlrVM8BvmZ9s

Yes, I think that sounds like a better approach.

Now, that I better understand what this revision is trying to do my general feedback would be: if we fix a false positive, it would be nice to have a test for it!

[lgrey]

Updated with test now that https://reviews.llvm.org/D133126 has landed.

I originally wanted the Foundation part to be a different patch, but it makes more sense to do it at once, since the scanning logic has to change anyway.

[yln]

compiler-rt/lib/lsan/lsan_common_mac.cpp

29–30

Can we get rid of this #ifdef here? I think we can drop support for compiling with SDK versions <= macOS 10.9 by now.
Maybe even inline the usage of the macros altogether?

38

Slight preference to make this an enum class and drop the k prefix:

enum class RegionKind {
  None = 0,
....

scan_state.seen_regions |= RegionKind::AllocOnce;

[lgrey]

enum -> enum class, remove memory tag aliases, clang-format

compiler-rt/lib/lsan/lsan_common_mac.cpp

38

Done (but a little uglier now since it needs casts if that makes a difference).

[yln]

compiler-rt/lib/lsan/lsan_common_mac.cpp

46

Would making this a SeenRegion field get rid of the casts?

[yln]

LGTM, after my final nit. Let's give others a few days to chime in.

[lgrey]

compiler-rt/lib/lsan/lsan_common_mac.cpp

46

Only if we implement operator| on it and do the casts in there. Given the number of uses it seems like a wash to me, happy to go with whichever you prefer.

[yln]

compiler-rt/lib/lsan/lsan_common_mac.cpp

46

Ahh, sorry I didn't realize this complication when suggesting using the enum class. TIL that enum classes and bitwise flags don't mesh well out of the box.

Since we are already halfway there, something like this should work, right?

inline AnimalFlags operator|(AnimalFlags a, AnimalFlags b)
{
    return static_cast<AnimalFlags>(static_cast<int>(a) | static_cast<int>(b));
}

Thank you and apologies for the detour.

[lgrey]

Add operator| and operator|=, remove casting

[lgrey]

I'll merge this later today if nobody has objections.

[lgrey]

Format

compiler-rt/lib/lsan/lsan_common_mac.cpp

 #include "lsan_allocator.h"
 
 #include <pthread.h>
 
 #include <mach/mach.h>
 
 // Only introduced in Mac OS X 10.9.
 #ifdef VM_MEMORY_OS_ALLOC_ONCE
 static const int kSanitizerVmMemoryOsAllocOnce = VM_MEMORY_OS_ALLOC_ONCE;
+static const int kSanitizerVmMemoryLibdispatch = VM_MEMORY_LIBDISPATCH;
 #else
 static const int kSanitizerVmMemoryOsAllocOnce = 73;
+static const int kSanitizerVmMemoryLibdispatch = 74;
 #endif
 
 namespace __lsan {
 
 typedef struct {
   int disable_counter;
   u32 current_thread_id;
   AllocatorCache cache;
 } thread_local_data_t;
 
 static pthread_key_t key;
 static pthread_once_t key_once = PTHREAD_ONCE_INIT;
 
 // The main thread destructor requires the current thread id,
 // so we can't destroy it until it's been used and reset to invalid tid
 void restore_tid_data(void *ptr) {
   thread_local_data_t *data = (thread_local_data_t *)ptr;
   if (data->current_thread_id != kInvalidTid)
     pthread_setspecific(key, data);
 }
 

 }
 
 void ProcessPlatformSpecificAllocations(Frontier *frontier) {
   vm_address_t address = 0;
   kern_return_t err = KERN_SUCCESS;
 
   InternalMmapVectorNoCtor<RootRegion> const *root_regions = GetRootRegions();
 
+  bool saw_alloc_once = false;
+  bool saw_libdispatch = false;
   while (err == KERN_SUCCESS) {
     vm_size_t size = 0;
     unsigned depth = 1;
     struct vm_region_submap_info_64 info;
     mach_msg_type_number_t count = VM_REGION_SUBMAP_INFO_COUNT_64;
     err = vm_region_recurse_64(mach_task_self(), &address, &size, &depth,
                                (vm_region_info_t)&info, &count);
 
     uptr end_address = address + size;
-
-    // libxpc stashes some pointers in the Kernel Alloc Once page,
-    // make sure not to report those as leaks.
+    bool scan_region = false;
     if (info.user_tag == kSanitizerVmMemoryOsAllocOnce) {
+      scan_region = true;
+      saw_alloc_once = true;
+    } else if (info.user_tag == kSanitizerVmMemoryLibdispatch) {
+      scan_region = true;
+      saw_libdispatch = true;
+    }
+
+    if (scan_region) {
       ScanRangeForPointers(address, end_address, frontier, "GLOBAL",
                            kReachable);
+    }
 
-      // Recursing over the full memory map is very slow, break out
-      // early if we don't need the full iteration.
-      if (!flags()->use_root_regions || !root_regions->size())
-        break;
-    }
+    if (saw_alloc_once && saw_libdispatch &&
+        (flags()->use_root_regions || !root_regions->size()))
 
     // This additional root region scan is required on Darwin in order to
     // detect root regions contained within mmap'd memory regions, because
     // the Darwin implementation of sanitizer_procmaps traverses images
     // as loaded by dyld, and not the complete set of all memory regions.
     //
     // TODO(fjricci) - remove this once sanitizer_procmaps_mac has the same
     // behavior as sanitizer_procmaps_linux and traverses all memory regions