Emit VMRegions for macOS when doing process memory dumps.

components/tracing/common/process_metrics_memory_dump_provider.cc

Index: components/tracing/common/process_metrics_memory_dump_provider.cc
diff --git a/components/tracing/common/process_metrics_memory_dump_provider.cc b/components/tracing/common/process_metrics_memory_dump_provider.cc
index 593755bada11899c83bd93f32689d819cd09fae7..20db847d3138f044a041feb74ac67f98ab039280 100644
--- a/components/tracing/common/process_metrics_memory_dump_provider.cc
+++ b/components/tracing/common/process_metrics_memory_dump_provider.cc
@@ -24,6 +24,13 @@
 #include "base/trace_event/process_memory_totals.h"
 #include "build/build_config.h"
 
+#if defined(OS_MACOSX)
+#include <libproc.h>
+#include <mach/mach.h>
+#include <mach/mach_vm.h>
+#include <sys/param.h>
+#endif  // defined(OS_MACOSX)
+
 namespace tracing {
 
 namespace {
@@ -219,6 +226,86 @@ bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
 }
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
+#if defined(OS_MACOSX)
+bool ProcessMetricsMemoryDumpProvider::DumpProcessMemoryMaps(
+    const base::trace_event::MemoryDumpArgs& args,
+    base::trace_event::ProcessMemoryDump* pmd) {
+  int pid = getpid();

[Mark Mentovai, 2017/01/31 17:49:21]

Optional: lately, I’ve enjoyed marking things like this that ought not change as
“const”.

[erikchen, 2017/01/31 20:01:18]

Done.

+  uint64_t kPageSize = 4096;

[Mark Mentovai, 2017/01/31 17:49:21]

Don’t define this yourself. Use PAGE_SIZE.

[erikchen, 2017/01/31 20:01:18]

Done.

+  using VMRegion = base::trace_event::ProcessMemoryMaps::VMRegion;
+  typedef vm_region_submap_info_64 RegionInfo;

[Mark Mentovai, 2017/01/31 17:49:21]

Since you used new-style “using” on the line before this one, why did you go
back to old-school “typedef” here?

[erikchen, 2017/01/31 20:01:17]

*cough* I copy pasted some code.

+  mach_vm_address_t address = 0;
+  mach_vm_size_t vmsize = 0;
+  natural_t depth = 0;
+  RegionInfo vminfo;
+  mach_msg_type_number_t count = sizeof(RegionInfo);
+  kern_return_t kr = KERN_SUCCESS;

[Mark Mentovai, 2017/01/31 17:49:21]

You can move this inside the loop. There’s no need to declare or initialize it
separately from the mach_vm_region_recurse() call.

[erikchen, 2017/01/31 20:01:17]

Done.

+  while (true) {
+    kr = mach_vm_region_recurse(mach_task_self(), &address, &vmsize, &depth,
+                                (vm_region_info_t)&vminfo, &count);

[Mark Mentovai, 2017/01/31 17:49:21]

Use cplusplus<style>(casts).

[erikchen, 2017/01/31 20:01:17]

Done.

+    if (kr != KERN_SUCCESS)

[Mark Mentovai, 2017/01/31 17:49:21]

I think that this should be

  if (kr == KERN_INVALID_ADDRESS)  // nothing else left
    break;
  if (kr != KERN_SUCCESS)  // something bad
    return false;

[erikchen, 2017/01/31 20:01:17]

Done.

+      break;
+
+    if ((int)vminfo.is_submap) {

[Mark Mentovai, 2017/01/31 17:49:21]

Lose the cast.

[erikchen, 2017/01/31 20:01:18]

Done.

+      ++depth;
+      continue;
+    }
+
+    VMRegion region;
+
+    // See vm_map_region_walk and vm_map_region_look_for_page in
+    // osfmk/vm/vm_map.c for more details.
+    if (vminfo.share_mode == SM_EMPTY || vminfo.share_mode == SM_LARGE_PAGE) {

[Mark Mentovai, 2017/01/31 17:49:21]

Is there really nothing that we can fill out for SM_LARGE_PAGE?

[erikchen, 2017/01/31 20:01:17]

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?

+    } else if (vminfo.share_mode == SM_COW) {
+      region.byte_stats_private_dirty_resident =
+          vminfo.pages_shared_now_private * kPageSize;
+      region.byte_stats_shared_clean_resident =
+          (vminfo.pages_resident - vminfo.pages_shared_now_private) *
+          kPageSize;
+    } else if (vminfo.share_mode == SM_PRIVATE) {
+      region.byte_stats_private_dirty_resident =
+          vminfo.pages_dirtied * kPageSize;
+      region.byte_stats_private_clean_resident =
+          (vminfo.pages_resident - vminfo.pages_dirtied) * kPageSize;
+    } else if (vminfo.share_mode == SM_SHARED ||
+               vminfo.share_mode == SM_PRIVATE_ALIASED ||
+               vminfo.share_mode == SM_TRUESHARED ||
+               vminfo.share_mode == SM_SHARED_ALIASED) {
+      region.byte_stats_shared_dirty_resident =
+          vminfo.pages_dirtied * kPageSize;
+      region.byte_stats_shared_clean_resident =
+          (vminfo.pages_resident - vminfo.pages_dirtied) * kPageSize;
+    } else {
+      NOTREACHED();
+    }
+
+    if (vminfo.protection & VM_PROT_READ)
+      region.protection_flags |= VMRegion::kProtectionFlagsRead;
+    if (vminfo.protection & VM_PROT_WRITE)
+      region.protection_flags |= VMRegion::kProtectionFlagsWrite;
+    if (vminfo.protection & VM_PROT_EXECUTE)
+      region.protection_flags |= VMRegion::kProtectionFlagsExec;
+
+    char buffer[MAXPATHLEN];
+    int length = proc_regionfilename(pid, address, buffer, MAXPATHLEN);

[Mark Mentovai, 2017/01/31 17:49:21]

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.

[erikchen, 2017/01/31 20:01:17]

Noted. We can add more information when we decide we need it.

+    if (length != 0)
+      region.mapped_file = std::string(buffer, length);

[Mark Mentovai, 2017/01/31 17:49:21]

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.

[erikchen, 2017/01/31 20:01:18]

Done.

+
+    region.byte_stats_swapped = vminfo.pages_swapped_out * kPageSize;
+    region.start_address = address;
+    region.size_in_bytes = vmsize;
+    pmd->process_mmaps()->AddVMRegion(region);
+
+    address = address + vmsize;

[Mark Mentovai, 2017/01/31 17:49:21]

+=

[erikchen, 2017/01/31 20:01:17]

Done.

+    if (address == 0u)

[Mark Mentovai, 2017/01/31 17:49:21]

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().

[erikchen, 2017/01/31 20:01:17]

Done.

+      break;
+  }
+
+  pmd->set_has_process_mmaps();
+  return true;
+}
+#endif  // defined(OS_MACOSX)
+
 // static
 void ProcessMetricsMemoryDumpProvider::RegisterForProcess(
     base::ProcessId process) {
@@ -267,11 +354,11 @@ bool ProcessMetricsMemoryDumpProvider::OnMemoryDump(
     base::trace_event::ProcessMemoryDump* pmd) {
   bool res = DumpProcessTotals(args, pmd);
 
-#if defined(OS_LINUX) || defined(OS_ANDROID)
+#if defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX)
   if (args.level_of_detail ==
       base::trace_event::MemoryDumpLevelOfDetail::DETAILED)
     res &= DumpProcessMemoryMaps(args, pmd);
-#endif
+#endif  // defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_MACOSX)
   return res;
 }