This is an archive of the discontinued LLVM Phabricator instance.

Make sure to scan mmap'd memory regions for root pointers on OS X
ClosedPublic

Authored by fjricci on Apr 18 2017, 2:22 PM.

Details

Summary

In the general case, we only need to check for root regions inside
the memory map returned by procmaps. However, on Darwin,
we also need to check inside mmap'd regions, which aren't returned
in the list of modules we get from procmaps.

This patch refactors memory region scanning on darwin to reduce
code duplication with the kernel alloc once page scan.

Event Timeline

fjricci created this revision.Apr 18 2017, 2:22 PM
kubamracek added inline comments.Apr 18 2017, 3:51 PM
lib/lsan/lsan_common_mac.cc
135–159

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.

fjricci added inline comments.Apr 19 2017, 7:50 AM
lib/lsan/lsan_common_mac.cc
135–159

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.

fjricci added inline comments.Apr 19 2017, 7:55 AM
lib/lsan/lsan_common_mac.cc
135–159

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.

kubamracek added inline comments.Apr 19 2017, 9:45 AM
lib/lsan/lsan_common_mac.cc
135–159

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.

fjricci added inline comments.Apr 19 2017, 9:53 AM
lib/lsan/lsan_common_mac.cc
135–159

I don't think that will change the performance at all, but perhaps the design here isn't super clear.

ProcessMemoryRegion is called (1 + num_root_regions) times, but it's only called with AllocOncePageCb one time. So there should be no duplication or caching benefit there.

I do see now how this could become slow if a large number of root regions was registered, since we'll iterate through all the pages for each root region. Perhaps I should refactor this such that ProcessMemoryRegion is only called once, and for each block does the VM_MEMORY_OS_ALLOC_ONCE check, in addition to iterating through the set of registered root regions to check if any are a match.

I think it comes down to whether we'd rather:

  1. Iterate through the root regions once, and on each iteration, traverse the list of memory regions
  2. Iterate through the list of memory regions once, and on each iteration run through all of the root regions.

My assumption is that 1) is simpler, but slower (and is what this patch does). 2) should be quite a bit more performant, but will also be a bit messier.

I'll put up a diff for 2) but save a copy of this one in case that turns out to be too ugly.

kubamracek added inline comments.Apr 19 2017, 10:11 AM
lib/lsan/lsan_common_mac.cc
135–159

Right, my suggestion is not to address performance, but to improve readability, and to make it clear what each function does. Currently, the name ProcessMemoryRegion doesn't suggest why we should be iterating over memory chunks with vm_region_recurse_64.

In any case, if the purpose of this loop is to find the alloc once region, we should have a separate function that does just that and nothing else.

fjricci updated this revision to Diff 95774.Apr 19 2017, 10:20 AM

Rework to only recurse memory regions once

This actually turns out to be relatively clean, I think.

fjricci updated this revision to Diff 95775.Apr 19 2017, 10:23 AM

Remove extra function definition

kubamracek added inline comments.Apr 19 2017, 10:33 AM
lib/lsan/lsan_common_mac.cc
149–156

Why is this needed again? I thought that we only need to account for the VM_MEMORY_OS_ALLOC_ONCE region.

Are we doing something like that on Linux? Why do we need to scan all mmap'd regions on Darwin only?

fjricci added inline comments.Apr 19 2017, 10:35 AM
lib/lsan/lsan_common_mac.cc
149–156

It appears that the Linux implementation of MemoryMappingLayout from sanitizer_procmaps includes mmap'd regions, while the Darwin implementation does not. I'm not sure whether that's because mmap-ing works differently on Darwin, or if it's a result of the differences between how we generate the module listing.

Yeah, it appears that sanitizer_procmaps_mac iterates using _dyld_get_image_vmaddr_slide, which will iterate over only images, as opposed to the vm_region_recurse_64 which iterates over all memory regions. The linux implementation just reads straight out of /proc/self/maps.

kubamracek added inline comments.Apr 19 2017, 10:47 AM
lib/lsan/lsan_common_mac.cc
149–156

I see. MemoryMappingLayout on Linux parses /proc/self/maps, which includes mmap regions, but Darwin uses dyld APIs to only iterate through modules.

For the moment, I think this is fine, but we should make a comment about the difference here. Ultimately, it would be better to converge the implementations of MemoryMappingLayout to do the same.

fjricci added inline comments.Apr 19 2017, 11:29 AM
lib/lsan/lsan_common_mac.cc
149–156

Ok. I'll make a comment and put looking into unifying the behavior on my list of things to do.

fjricci updated this revision to Diff 95789.Apr 19 2017, 11:33 AM

Update comment and add TODO

alekseyshl accepted this revision.Apr 19 2017, 2:20 PM
This revision is now accepted and ready to land.Apr 19 2017, 2:20 PM
This revision was automatically updated to reflect the committed changes.
kubamracek added inline comments.Apr 19 2017, 2:26 PM
lib/lsan/lsan_common.cc
86

Can this be static since we now have GetRootRegions?

291–308

Can this be const RootRegion & instead? The rest of compiler-rt seems to heavily prefer const X & rather than X const &. Same below.

Done in r300765