Move logic from memory_mac to allocator_interception_mac.

Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567
Review-Url: https://codereview.chromium.org/2649973003
Cr-Original-Commit-Position: refs/heads/master@{#446234}
Committed: https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea46ec096230506
Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446385}
Committed: https://chromium.googlesource.com/chromium/src/+/9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa

[erikchen, 2017-01-23 21:55:39]

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

[commit-bot: I haz the power, 2017-01-23 21:57:00]

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-01-23 22:02:53]

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

[commit-bot: I haz the power, 2017-01-23 22:02:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[erikchen, 2017-01-23 22:57:21]

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

[commit-bot: I haz the power, 2017-01-23 22:58:28]

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-01-24 00:36:49]

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

[commit-bot: I haz the power, 2017-01-24 00:36:50]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-24 01:09:41]

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

[erikchen, 2017-01-24 01:09:41]

primiano: Please review.

[erikchen, 2017-01-25 18:44:30]

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

[erikchen, 2017-01-25 18:44:31]

mark: Please review.

Original CL is https://codereview.chromium.org/2601573002/. primiano suggested
that I do some refactoring, and also break into smaller CLs.

[Mark Mentovai, 2017-01-25 18:50:28]

LGTM

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:19: }
// namespace base

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.mm (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:66: kern_return_t result =
mach_vm_region(
clang-format, huh? Not better, but if that’s what wrote this, then I guess we
don’t have to talk about it.

[erikchen, 2017-01-25 19:05:45]

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:19: }
On 2017/01/25 18:50:27, Mark Mentovai wrote:
> // namespace base

Done.

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.mm (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:66: kern_return_t result =
mach_vm_region(
On 2017/01/25 18:50:27, Mark Mentovai wrote:
> clang-format, huh? Not better, but if that’s what wrote this, then I guess we
> don’t have to talk about it.

yesssir

[erikchen, 2017-01-25 19:05:49]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-01-25 19:05:49]

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/2649973003/#ps40001
(title: "Comments from mark.")

[commit-bot: I haz the power, 2017-01-25 19:06:16]

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

[Primiano Tucci (use gerrit), 2017-01-25 19:10:58]

https://codereview.chromium.org/2649973003/diff/20001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2649973003/diff/20001/base/BUILD.gn#newcode152
base/BUILD.gn:152: "allocator/allocator_interception_mac.h",
I think this should be in base/allocator_shim/BUILD.gn
Otherwise you  won't be able to use UncheckedMalloc/calloc in the shim because
of cyclic dependencies.

allocator_shim is a source_set and does not depend on base (while it's true the
other way round, base depends on it). 
As it is right now, you won't be able to use UncheckedMallocMac from
allocator_shim, which would defeat the point of this refactoring exercise :)

If the only base bits you need in interception_mac.mm are pure-headers (e.g.
logging.h) that should work.
for instance I see that allocator_shim.cc right now uses logging.h and that
seems to work.

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:10: namespace base {
+ namespace allocator {

[erikchen, 2017-01-25 19:26:28]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-01-25 19:41:06]

The fact that allocator_shim is a separate source_set, but contains TUs that
depend on base headers shows that it shouldn't be a separate source set. 

There are several constraints that cannot simultaneously be satisfied:
1) Refactor logic from base into base/allocator, without changing crash
signatures. 
2) Mirror structure of both base/process/memory.h interface, and base/allocator
desired directory structure across all platforms.
3) Keep existing dependency structure between base and allocator_shim [which I
claim is already violated]

[Primiano Tucci (use gerrit), 2017-01-25 21:14:35]

On 2017/01/25 19:41:06, erikchen wrote:
> The fact that allocator_shim is a separate source_set, but contains TUs that
> depend on base headers shows that it shouldn't be a separate source set. 

Ok I checked and realized I had a brainfart here. the things in the
allocator/build.gn are mainly relative to the unified_allocator_shim, which is
excluded  when use_experimental_allocator_shim == false.
This does no apply to this code. we want this code anyways,  similarly to what
happens to allocator_check.h and allocator_extension.
Ignore my previous comment. LGTM

[erikchen, 2017-01-25 22:03:42]

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2649973003/diff/20001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:10: namespace base {
On 2017/01/25 19:10:58, Primiano Tucci wrote:
> + namespace allocator {

Done.

[erikchen, 2017-01-25 22:03:46]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-01-25 22:03:46]

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/2649973003/#ps80001
(title: "Comments from primiano.")

[commit-bot: I haz the power, 2017-01-25 22:04:32]

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-01-26 01:37:25]

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

[commit-bot: I haz the power, 2017-01-26 01:37:26]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[erikchen, 2017-01-26 01:41:11]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-01-26 01:42:30]

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-01-26 05:37:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485394871357480,
"parent_rev": "c850f7de840cdb2c21ee10166a76c2acbf04cc01", "commit_rev":
"9550b71347ce9acbe2875fb75ea46ec096230506"}

[commit-bot: I haz the power, 2017-01-26 05:38:03]

Description was changed from

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567
==========

to

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567

Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446234}
Committed:
https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea4...
==========

[commit-bot: I haz the power, 2017-01-26 05:38:05]

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

[ajuma, 2017-01-26 16:33:58]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2658083002/ by ajuma@chromium.org.

The reason for reverting is: This seems to be causing compile to fail on the Mac
GPU FYI Asan bot, with error:
../../base/allocator/allocator_interception_mac.mm:320:3: error: use of
undeclared identifier 'DeprotectMallocZone'
  DeprotectMallocZone(zone, &reprotection_start, &reprotection_length,
  ^
1 error generated.

See
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel...
and
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel....

[Primiano Tucci (use gerrit), 2017-01-26 16:45:39]

On 2017/01/26 16:33:58, ajuma wrote:
> A revert of this CL (patchset #5 id:80001) has been created in
> https://codereview.chromium.org/2658083002/ by mailto:ajuma@chromium.org.
> 
> The reason for reverting is: This seems to be causing compile to fail on the
Mac
> GPU FYI Asan bot, with error:
> ../../base/allocator/allocator_interception_mac.mm:320:3: error: use of
> undeclared identifier 'DeprotectMallocZone'
>   DeprotectMallocZone(zone, &reprotection_start, &reprotection_length,
>   ^
> 1 error generated.
> 
> See
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel...
> and
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel....

I think actually the problem was not here but on the
https://codereview.chromium.org/2650363002 on top of this, which missed an #if
!defined(ADDRESS_SANITIZER)

Just curious how did you manage to revert this but not the one based on top of
it?

[ajuma, 2017-01-26 18:27:31]

On 2017/01/26 16:45:39, Primiano Tucci wrote:
> On 2017/01/26 16:33:58, ajuma wrote:
> > A revert of this CL (patchset #5 id:80001) has been created in
> > https://codereview.chromium.org/2658083002/ by mailto:ajuma@chromium.org.
> > 
> > The reason for reverting is: This seems to be causing compile to fail on the
> Mac
> > GPU FYI Asan bot, with error:
> > ../../base/allocator/allocator_interception_mac.mm:320:3: error: use of
> > undeclared identifier 'DeprotectMallocZone'
> >   DeprotectMallocZone(zone, &reprotection_start, &reprotection_length,
> >   ^
> > 1 error generated.
> > 
> > See
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel...
> > and
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel....
> 
> I think actually the problem was not here but on the
> https://codereview.chromium.org/2650363002 on top of this, which missed an #if
> !defined(ADDRESS_SANITIZER)
> 
> Just curious how did you manage to revert this but not the one based on top of
> it?

Hmm, good question. Looks like the revert just deleted the files added in this
CL, despite them being modified after this CL landed.

[erikchen, 2017-01-26 19:03:38]

On 2017/01/26 18:27:31, ajuma wrote:
> On 2017/01/26 16:45:39, Primiano Tucci wrote:
> > On 2017/01/26 16:33:58, ajuma wrote:
> > > A revert of this CL (patchset #5 id:80001) has been created in
> > > https://codereview.chromium.org/2658083002/ by mailto:ajuma@chromium.org.
> > > 
> > > The reason for reverting is: This seems to be causing compile to fail on
the
> > Mac
> > > GPU FYI Asan bot, with error:
> > > ../../base/allocator/allocator_interception_mac.mm:320:3: error: use of
> > > undeclared identifier 'DeprotectMallocZone'
> > >   DeprotectMallocZone(zone, &reprotection_start, &reprotection_length,
> > >   ^
> > > 1 error generated.
> > > 
> > > See
> > >
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel...
> > > and
> > >
> >
>
https://build.chromium.org/p/chromium.gpu.fyi/builders/Mac%20GPU%20ASAN%20Rel....
> > 
> > I think actually the problem was not here but on the
> > https://codereview.chromium.org/2650363002 on top of this, which missed an
#if
> > !defined(ADDRESS_SANITIZER)
> > 
> > Just curious how did you manage to revert this but not the one based on top
of
> > it?
> 
> Hmm, good question. Looks like the revert just deleted the files added in this
> CL, despite them being modified after this CL landed.

I'm relanding this CL, since it doesn't actually cause ASAN build errors. The
dependent CL https://codereview.chromium.org/2650363002/ causes errors.

[erikchen, 2017-01-26 19:03:45]

Description was changed from

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567

Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446234}
Committed:
https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea4...
==========

to

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567

Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446234}
Committed:
https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea4...
==========

[erikchen, 2017-01-26 19:03:53]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-01-26 19:04:19]

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-01-26 19:22:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485457433300010,
"parent_rev": "9a265dc92c9aabf095817219ca44f094c81cfdd5", "commit_rev":
"9dbabee0d6c79ecf51ba5cc5edc7cd7d02f6b4fa"}

[commit-bot: I haz the power, 2017-01-26 19:23:11]

Description was changed from

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567

Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446234}
Committed:
https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea4...
==========

to

==========
Move logic from memory_mac to allocator_interception_mac.

This CL is a refactor and has no intended behavior changes.

This CL moves the allocator interception logic from base/process/memory_mac.mm
to base/allocator/allocator_interception_mac.mm in preparation for an
implementation of the allocator shim.

BUG=665567

Review-Url: https://codereview.chromium.org/2649973003
Cr-Original-Commit-Position: refs/heads/master@{#446234}
Committed:
https://chromium.googlesource.com/chromium/src/+/9550b71347ce9acbe2875fb75ea4...
Review-Url: https://codereview.chromium.org/2649973003
Cr-Commit-Position: refs/heads/master@{#446385}
Committed:
https://chromium.googlesource.com/chromium/src/+/9dbabee0d6c79ecf51ba5cc5edc7...
==========

[commit-bot: I haz the power, 2017-01-26 19:23:13]

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