Update logic for emitting region information from dyld.

Update logic for emitting region information from dyld.

1. Use the slide to compute the segment load addresses.
2. Update the logic for combining regions from dyld and mach_vm_region_recurse.
  2a. Allow a vm region to be fully contained with a dyld region.
  2b. Skip vm regions that intersect dyld regions in the dyld shared cache.

BUG=677302
Review-Url: https://codereview.chromium.org/2696923002
Cr-Commit-Position: refs/heads/master@{#450757}
Committed: https://chromium.googlesource.com/chromium/src/+/99b21cc6a775c54d8df60f785356da3b21f054e2

[erikchen, 2017-02-14 21:59:51]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2017-02-14 21:59:52]

mark: Please review.

[erikchen, 2017-02-14 21:59:55]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 22:00:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2017-02-14 22:18:58]

LGTM

https://codereview.chromium.org/2696923002/diff/1/components/tracing/common/p...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/1/components/tracing/common/p...
components/tracing/common/process_metrics_memory_dump_provider.cc:294: // Avoid
emitting LINKEDIT regions in the dyld shared cache, since they
Perhaps just emit it one time, then? Show it as belonging to the shared cache
instead of any particular module?

[erikchen, 2017-02-14 22:33:47]

https://codereview.chromium.org/2696923002/diff/1/components/tracing/common/p...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/1/components/tracing/common/p...
components/tracing/common/process_metrics_memory_dump_provider.cc:294: // Avoid
emitting LINKEDIT regions in the dyld shared cache, since they
On 2017/02/14 22:18:58, Mark Mentovai wrote:
> Perhaps just emit it one time, then? Show it as belonging to the shared cache
> instead of any particular module?

Done.

[erikchen, 2017-02-14 22:33:51]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-14 22:33:51]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2696923002/#ps20001
(title: "Emit linkedit region exactly once.")

[commit-bot: I haz the power, 2017-02-14 22:34:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-02-14 22:43:48]

The CQ bit was unchecked by erikchen@chromium.org

[Mark Mentovai, 2017-02-14 22:45:06]

but LGTM either way

https://codereview.chromium.org/2696923002/diff/20001/components/tracing/comm...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/20001/components/tracing/comm...
components/tracing/common/process_metrics_memory_dump_provider.cc:303:
image_name = "LinkEditDyldSharedCache";
It’d be nice to use the actual proc_regionfilename of the dyld shared cache
here.

[erikchen, 2017-02-14 23:31:31]

Description was changed from

==========
Update logic for emitting region information from dyld.

Use the slide to compute the segment load addresses. Also update the logic for
combining regions from dyld and mach_vm_region_recurse.

BUG=677302
==========

to

==========
Update logic for emitting region information from dyld.

1. Use the slide to compute the segment load addresses.
2. Update the logic for combining regions from dyld and mach_vm_region_recurse.
  2a. Allow a vm region to be fully contained with a dyld region.
  2b. Skip vm regions that intersect dyld regions in the dyld shared cache.

BUG=677302
==========

[erikchen, 2017-02-14 23:31:58]

mark: I updated the combining logic to allow a vm region to be fully contained
within a dyld region, which happens in practice.

https://codereview.chromium.org/2696923002/diff/20001/components/tracing/comm...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/20001/components/tracing/comm...
components/tracing/common/process_metrics_memory_dump_provider.cc:303:
image_name = "LinkEditDyldSharedCache";
On 2017/02/14 22:45:06, Mark Mentovai wrote:
> It’d be nice to use the actual proc_regionfilename of the dyld shared cache
> here.

I went with "dyld shared cache combined __LINKEDIT", which is the name vmmap
gives this region. proc_regionfilename returns nothing.

[erikchen, 2017-02-14 23:32:01]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 23:33:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 01:30:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 01:30:55]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-02-15 03:38:20]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-15 03:38:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2696923002/#ps40001
(title: "Allow memory regions to be subsets of dyld regions.")

[commit-bot: I haz the power, 2017-02-15 03:39:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mark Mentovai, 2017-02-15 03:46:42]

That’s cool then. LGTM.

[commit-bot: I haz the power, 2017-02-15 04:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 04:38:12]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[erikchen, 2017-02-15 05:01:20]

erikchen@chromium.org changed reviewers:
+ primiano@chromium.org

[erikchen, 2017-02-15 05:01:20]

primiano: Please review.

[Primiano Tucci (use gerrit), 2017-02-15 09:54:32]

LGTM

https://codereview.chromium.org/2696923002/diff/40001/components/tracing/comm...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/40001/components/tracing/comm...
components/tracing/common/process_metrics_memory_dump_provider.cc:259: uint64_t
b_end_address = b.start_address + b.size_in_bytes;
You can halve the numbers of comparisons here. Assuming that the regions are
well formed (each end > start), you can just :
Return start_a < end_b && start_b < end_a 
(or if you prefer the negative logic is easier to read. 
 They don't overlap if (s_a >= e_b || s_b >= e_a)

[erikchen, 2017-02-15 17:26:21]

https://codereview.chromium.org/2696923002/diff/40001/components/tracing/comm...
File components/tracing/common/process_metrics_memory_dump_provider.cc (right):

https://codereview.chromium.org/2696923002/diff/40001/components/tracing/comm...
components/tracing/common/process_metrics_memory_dump_provider.cc:259: uint64_t
b_end_address = b.start_address + b.size_in_bytes;
On 2017/02/15 09:54:31, Primiano Tucci wrote:
> You can halve the numbers of comparisons here. Assuming that the regions are
> well formed (each end > start), you can just :
> Return start_a < end_b && start_b < end_a 
> (or if you prefer the negative logic is easier to read. 
>  They don't overlap if (s_a >= e_b || s_b >= e_a)

Done.

[erikchen, 2017-02-15 17:26:24]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-15 17:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2696923002/#ps80001
(title: "Fix comparison operator.")

[commit-bot: I haz the power, 2017-02-15 17:27:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 19:11:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487179584094900,
"parent_rev": "81f013144ccfc759101193455919b4c1d04c1715", "commit_rev":
"99b21cc6a775c54d8df60f785356da3b21f054e2"}

[commit-bot: I haz the power, 2017-02-15 19:11:54]

Description was changed from

==========
Update logic for emitting region information from dyld.

1. Use the slide to compute the segment load addresses.
2. Update the logic for combining regions from dyld and mach_vm_region_recurse.
  2a. Allow a vm region to be fully contained with a dyld region.
  2b. Skip vm regions that intersect dyld regions in the dyld shared cache.

BUG=677302
==========

to

==========
Update logic for emitting region information from dyld.

1. Use the slide to compute the segment load addresses.
2. Update the logic for combining regions from dyld and mach_vm_region_recurse.
  2a. Allow a vm region to be fully contained with a dyld region.
  2b. Skip vm regions that intersect dyld regions in the dyld shared cache.

BUG=677302

Review-Url: https://codereview.chromium.org/2696923002
Cr-Commit-Position: refs/heads/master@{#450757}
Committed:
https://chromium.googlesource.com/chromium/src/+/99b21cc6a775c54d8df60f785356...
==========

[commit-bot: I haz the power, 2017-02-15 19:11:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/99b21cc6a775c54d8df60f785356...