Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601193002:
Emit VMRegions for macOS when doing process memory dumps. (Closed)
DescriptionEmit VMRegions for macOS when doing process memory dumps.
Use mach_vm_region_recurse to recurse through all memory regions and emit the
relevant stats.
BUG=677302
Review-Url: https://codereview.chromium.org/2601193002
Cr-Commit-Position: refs/heads/master@{#448048}
Committed: https://chromium.googlesource.com/chromium/src/+/230f8e308399900f340cc2254ce2e3aef08fb2d9
Patch Set 1 #Patch Set 2 : Clean up. #Patch Set 3 : Add test fix code. #Patch Set 4 : More cleanup. #Patch Set 5 : Don't use symbol on windows. #
Total comments: 7
Patch Set 6 : Comments from mark, primiano. #Patch Set 7 : Clean up. #Patch Set 8 : Update test. #
Total comments: 24
Patch Set 9 : Comments from mark. #
Total comments: 2
Patch Set 10 : Remove debugging. #
Total comments: 6
Patch Set 11 : Rebase. #Patch Set 12 : Switch to VM_REGION_EXTENDED_INFO. Comments from primiano. #Patch Set 13 : Clean up. #Patch Set 14 : Switch back to region recurse. #
Messages
Total messages: 45 (32 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
erikchen@chromium.org changed reviewers: + mark@chromium.org, primiano@chromium.org
primiano, mark: Please review.
https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:267: if (!strcmp(seg->segname, "__TEXT")) { You can use SEG_TEXT as a symbolic name for __TEXT. Also, I like to strcmp == 0 instead of !strcmp. https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:270: // It's not clear what "protection_flags" means for an entire They’re the access bits set when the region is initially mapped. Sure, you can change those bits up to what’s allowed by maxprot (or remap new pages in their place even beyond maxprot), but all you’re seeing here is the mapped Mach-O image. These bits that you’re looking at are just what the kernel or dyld use to set the region’s initial prot and maxprot. https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:285: reinterpret_cast<const char*>(load_cmd) + load_cmd->cmdsize); Avoid running beyond header->sizeofcmds, bearing in mind that the entire segment command that you interpret needs to fit within sizeofcmds. https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:289: region.start_address = reinterpret_cast<uint64_t>(header); For this to be valid, __TEXT needs to be at offset 0, which it normally should be. Check on seg->fileoff. Refer to https://chromium.googlesource.com/crashpad/crashpad/+/bb7d249d65a1edd965499b7.... https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:292: pmd->set_has_process_mmaps(); This only gets things that dyld knows about, including the main executable, but excluding dyld itself. You can add dyld fairly easily, but even so, this is somewhat short of everything that might be mapped. You can mmap() your own file and this list won’t contain it, for example. For that matter, you’re only looking at __TEXT, although almost every module will have more than just that segment mapped. Even if just looking at the dyld interface, you can add a new region (from this component’s perspective) for each segment in a mapped Mach-O image. The alternative would grab everything that’s been mapped, which is more like what vmmap would tell you. That may be more in line with what’s needed. The Linux implementation above snarfs smaps, which is going to see everything that’s been mapped. But on the other hand, Linux has no (standard) way to grab just the code modules that its loader knows about, where macOS provides this dyld API that you’re using. How is this code used? Should we grab all loaded modules, or just loaded code that the loader’s aware of? You can grab all mapped regions by walking the process’ VM map with Mach calls (mach_vm_region_recurse()), and with each region, determine if it’s mapped by calling proc_pidinfo() to fetch PROC_PIDREGIONPATHINFO and perhaps PROC_PIDREGIONINFO. (If you only need PROC_PIDREGIONPATHINFO, try the proc_regionfilename() wrapper.) https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:291: if (region.mapped_file.find("components_unittests") != std::string::npos) Might be less fragile if you used basename(_NSGetExecutablePath()) instead of hard-coding.
https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/80001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:292: pmd->set_has_process_mmaps(); On 2017/01/03 20:07:46, Mark Mentovai wrote: > This only gets things that dyld knows about, including the main executable, but > excluding dyld itself. You can add dyld fairly easily, but even so, this is > somewhat short of everything that might be mapped. You can mmap() your own file > and this list won’t contain it, for example. > > For that matter, you’re only looking at __TEXT, although almost every module > will have more than just that segment mapped. Even if just looking at the dyld > interface, you can add a new region (from this component’s perspective) for each > segment in a mapped Mach-O image. > > The alternative would grab everything that’s been mapped, which is more like > what vmmap would tell you. > > That may be more in line with what’s needed. The Linux implementation above > snarfs smaps, which is going to see everything that’s been mapped. But on the > other hand, Linux has no (standard) way to grab just the code modules that its > loader knows about, where macOS provides this dyld API that you’re using. How is > this code used? Should we grab all loaded modules, or just loaded code that the > loader’s aware of? > > You can grab all mapped regions by walking the process’ VM map with Mach calls > (mach_vm_region_recurse()), and with each region, determine if it’s mapped by > calling proc_pidinfo() to fetch PROC_PIDREGIONPATHINFO and perhaps > PROC_PIDREGIONINFO. (If you only need PROC_PIDREGIONPATHINFO, try the > proc_regionfilename() wrapper.) On Linux/Android we deliberately decided to grab all mappings (not just code) as the purpose of this is to debug the state of memory giving a snapshot of what the OS sees. Historically this helped us to pinpoint regressions of the form: - Oh look there are some extra .ttf files mapped, this regression has something to do with fonts. - Oh look there are a bunch of anonymous regions of size 512*512*4, these are very likely some cc tiles. Not sure how complicated and invasive would be doing the same on MacOS. If possible would be desirable
Description was changed from ========== Emit VMRegions for macOS when doing process memory dumps. Use dyld API to determine the load locations of the images and their sizes. BUG=677302 ========== to ========== Emit VMRegions for macOS when doing process memory dumps. Use mach_vm_region_recurse to recurse through all memory regions and emit the relevant stats. BUG=677302 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mark, primiano: Please review.
https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:233: int pid = getpid(); Optional: lately, I’ve enjoyed marking things like this that ought not change as “const”. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:234: uint64_t kPageSize = 4096; Don’t define this yourself. Use PAGE_SIZE. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:236: typedef vm_region_submap_info_64 RegionInfo; Since you used new-style “using” on the line before this one, why did you go back to old-school “typedef” here? https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:242: kern_return_t kr = KERN_SUCCESS; You can move this inside the loop. There’s no need to declare or initialize it separately from the mach_vm_region_recurse() call. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:245: (vm_region_info_t)&vminfo, &count); Use cplusplus<style>(casts). https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:246: if (kr != KERN_SUCCESS) I think that this should be if (kr == KERN_INVALID_ADDRESS) // nothing else left break; if (kr != KERN_SUCCESS) // something bad return false; https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:249: if ((int)vminfo.is_submap) { Lose the cast. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:258: if (vminfo.share_mode == SM_EMPTY || vminfo.share_mode == SM_LARGE_PAGE) { Is there really nothing that we can fill out for SM_LARGE_PAGE? https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:290: int length = proc_regionfilename(pid, address, buffer, MAXPATHLEN); A problem here is that you get a couple of gigantic regions for the dyld shared cache, instead of seeing the individual modules that it has cached. Of course it’s correct in the sense that the shared cache is what was mapped, but it might not be the friendliest thing for developers to deal with. This is OK, but if you or someone else wants to come back later and slurp up the images from dyld (as your previous version did) and use that stuff to fill out loaded module information and present them as smaller regions, I’d be OK with that too. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:292: region.mapped_file = std::string(buffer, length); I don’t know if the compiler can always optimize away the construction and assignment with this verbiage. I’d normally use region.mapped_file.assign(buffer, length) to be sure. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:299: address = address + vmsize; += https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:300: if (address == 0u) I think you want to test whether address overflowed to anything, not necessarily just to 0. This is handy: if (__builtin_add_overflow(address, vmsize, &address)) break; or #include <os/overflow.h> to use os_add_overflow() which wraps __builtin_add_overflow(). or #include "base/numeric/safe_math.h", and base::CheckAdd(), which also wraps __builtin_add_overflow() and returns a base::CheckedNumeric on which you can call IsValid() and ValueOrDie().
mark: Please review. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:233: int pid = getpid(); On 2017/01/31 17:49:21, Mark Mentovai wrote: > Optional: lately, I’ve enjoyed marking things like this that ought not change as > “const”. Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:234: uint64_t kPageSize = 4096; On 2017/01/31 17:49:21, Mark Mentovai wrote: > Don’t define this yourself. Use PAGE_SIZE. Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:236: typedef vm_region_submap_info_64 RegionInfo; On 2017/01/31 17:49:21, Mark Mentovai wrote: > Since you used new-style “using” on the line before this one, why did you go > back to old-school “typedef” here? *cough* I copy pasted some code. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:242: kern_return_t kr = KERN_SUCCESS; On 2017/01/31 17:49:21, Mark Mentovai wrote: > You can move this inside the loop. There’s no need to declare or initialize it > separately from the mach_vm_region_recurse() call. Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:245: (vm_region_info_t)&vminfo, &count); On 2017/01/31 17:49:21, Mark Mentovai wrote: > Use cplusplus<style>(casts). Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:246: if (kr != KERN_SUCCESS) On 2017/01/31 17:49:21, Mark Mentovai wrote: > I think that this should be > > if (kr == KERN_INVALID_ADDRESS) // nothing else left > break; > if (kr != KERN_SUCCESS) // something bad > return false; Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:249: if ((int)vminfo.is_submap) { On 2017/01/31 17:49:21, Mark Mentovai wrote: > Lose the cast. Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:258: if (vminfo.share_mode == SM_EMPTY || vminfo.share_mode == SM_LARGE_PAGE) { On 2017/01/31 17:49:21, Mark Mentovai wrote: > Is there really nothing that we can fill out for SM_LARGE_PAGE? Hm. In https://llvm.org/svn/llvm-project/lldb/trunk/tools/debugserver/source/MacOSX/..., they treat this the same as SM_PRIVATE. Looking at ./osfmk/vm/vm_map.c, it looks like not all the fields we care about get populated [like the dirty field]. I've changed the logic to use the same as SM_PRIVATE. Sound reasonable? https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:290: int length = proc_regionfilename(pid, address, buffer, MAXPATHLEN); On 2017/01/31 17:49:21, Mark Mentovai wrote: > A problem here is that you get a couple of gigantic regions for the dyld shared > cache, instead of seeing the individual modules that it has cached. Of course > it’s correct in the sense that the shared cache is what was mapped, but it might > not be the friendliest thing for developers to deal with. > > This is OK, but if you or someone else wants to come back later and slurp up the > images from dyld (as your previous version did) and use that stuff to fill out > loaded module information and present them as smaller regions, I’d be OK with > that too. Noted. We can add more information when we decide we need it. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:292: region.mapped_file = std::string(buffer, length); On 2017/01/31 17:49:21, Mark Mentovai wrote: > I don’t know if the compiler can always optimize away the construction and > assignment with this verbiage. I’d normally use > region.mapped_file.assign(buffer, length) to be sure. Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:299: address = address + vmsize; On 2017/01/31 17:49:21, Mark Mentovai wrote: > += Done. https://codereview.chromium.org/2601193002/diff/140001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:300: if (address == 0u) On 2017/01/31 17:49:21, Mark Mentovai wrote: > I think you want to test whether address overflowed to anything, not necessarily > just to 0. This is handy: > > if (__builtin_add_overflow(address, vmsize, &address)) > break; > > or #include <os/overflow.h> to use os_add_overflow() which wraps > __builtin_add_overflow(). > > or #include "base/numeric/safe_math.h", and base::CheckAdd(), which also wraps > __builtin_add_overflow() and returns a base::CheckedNumeric on which you can > call IsValid() and ValueOrDie(). Done.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2601193002/diff/160001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/160001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:259: if (vminfo.share_mode == SM_LARGE_PAGE) How can share_mode be both SM_EMPTY and SM_LARGE_PAGE?
https://codereview.chromium.org/2601193002/diff/160001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/160001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:259: if (vminfo.share_mode == SM_LARGE_PAGE) On 2017/01/31 20:29:13, Mark Mentovai wrote: > How can share_mode be both SM_EMPTY and SM_LARGE_PAGE? whoops, left over debugging that wasn't even in the right place. removed.
Cool, LGTM.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM, just one minor comment https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:258: if (vminfo.share_mode == SM_EMPTY) { a switch(vminfo.share_mode ) might have been slightly more elegant (would have avoided this empty block for instance). Although, it's a minor style thing, so if there are no other changes planned to this cl just ignore this comment and tick commit. https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:291: int length = proc_regionfilename(pid, address, buffer, MAXPATHLEN); I looked at the sources of this one and found really *enjoyable* the fact that proc_regionfilename takes a max-len argument but just returns ENOMEM if that argument is < MAPATHLEN [1] :) [1] https://opensource.apple.com/source/Libc/Libc-498/darwin/libproc.c https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:300: if (__builtin_add_overflow(address, vmsize, &address)) can this be done with base/numerics/safe_math.h? Didn't find any other precedent of using __builtin_add_ (other than in base/numerics) Fine if for whatever reason is too much of a pain or requires refactorings. https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider_unittest.cc (right): https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider_unittest.cc:278: TEST(ProcessMetricsMemoryDumpProviderTest, TestMachOReading) { \o/ Look a test! thanks, you rock.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:258: if (vminfo.share_mode == SM_EMPTY) { On 2017/02/01 00:03:57, Primiano Tucci wrote: > a switch(vminfo.share_mode ) might have been slightly more elegant (would have > avoided this empty block for instance). > Although, it's a minor style thing, so if there are no other changes planned to > this cl just ignore this comment and tick commit. Done. https://codereview.chromium.org/2601193002/diff/180001/components/tracing/com... components/tracing/common/process_metrics_memory_dump_provider.cc:300: if (__builtin_add_overflow(address, vmsize, &address)) On 2017/02/01 00:03:57, Primiano Tucci wrote: > can this be done with base/numerics/safe_math.h? > Didn't find any other precedent of using __builtin_add_ (other than in > base/numerics) > Fine if for whatever reason is too much of a pain or requires refactorings. Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2601193002/#ps260001 (title: "Switch back to region recurse.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1486146471286150,
"parent_rev": "020cc798ffc13c3b851310f3925ab3d2661477b1", "commit_rev":
"230f8e308399900f340cc2254ce2e3aef08fb2d9"}
Message was sent while issue was closed.
Description was changed from ========== Emit VMRegions for macOS when doing process memory dumps. Use mach_vm_region_recurse to recurse through all memory regions and emit the relevant stats. BUG=677302 ========== to ========== Emit VMRegions for macOS when doing process memory dumps. Use mach_vm_region_recurse to recurse through all memory regions and emit the relevant stats. BUG=677302 Review-Url: https://codereview.chromium.org/2601193002 Cr-Commit-Position: refs/heads/master@{#448048} Committed: https://chromium.googlesource.com/chromium/src/+/230f8e308399900f340cc2254ce2... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/230f8e308399900f340cc2254ce2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
