[PATCH] D32190: Make sure to scan mmap'd memory regions for root pointers on OS X

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek added inline comments.


================
Comment at: lib/lsan/lsan_common_mac.cc:150-156
   while (err == KERN_SUCCESS) {
     struct vm_region_submap_info_64 info;
     err = vm_region_recurse_64(port, &address, &size, &depth,
                                (vm_region_info_t)&info, &count);
-    if (info.user_tag == VM_MEMORY_OS_ALLOC_ONCE) {
-      ScanRangeForPointers(address, address + size, frontier,
-                           "GLOBAL", kReachable);
-      return;
-    }
+    callback(frontier, address, size, info, arg);
     address += size;
   }
----------------
fjricci wrote:
> fjricci wrote:
> > kubamracek wrote:
> > > How many times is this called?  This should really be called only once per the whole scan, and it's hard to see if that's the case or not.
> > > 
> > > The naming is weird: `ProcessMemoryRegion` suggests that this processes some specific region, but in fact, this function *scans* for a new region.  I don't follow why this is called from ProcessPlatformSpecificRootRegion.
> > This would be called once when checking for the kernel alloc once page, and once when processing the root regions. I could probably change it to do both in one loop, if that's preferable, but it would require changing the ProcessPlatformSpecificAllocations API to take root region pointers (probably not a big deal, since only mac uses that API currently).
> > 
> > It needs to be called from ProcessPlatformSpecificRootRegion to find root regions contained within mmap'd memory regions.
> Actually that refactor would probably be even more complicated than this version, as we'd need to encapsulate the functionality of `ProcessRootRegions` (which iterates over the root regions, scanning each one). Currently we avoid duplicating this behavior by doing the platform-specific scan inside the call tree of that function.
Can you try doing something else then:  Create a `GetKernelAllocOnceRegion`, which will return the start and size of the VM_MEMORY_OS_ALLOC_ONCE region, and then it will cache the result; 2nd calls to this will just return the cached values.  Then you can call this function as many times as you need.


https://reviews.llvm.org/D32190