mac: Several minor fixes to allocator shim.

mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
Review-Url: https://codereview.chromium.org/2727463002
Cr-Commit-Position: refs/heads/master@{#459964}
Committed: https://chromium.googlesource.com/chromium/src/+/2f9bf057e1227bcee1d24e4b619894233946d26c

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

Description was changed from

==========
mac: Several minor fixes to allocator shim.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.

BUG=693237
==========

[erikchen, 2017-02-28 23:33:08]

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

[commit-bot: I haz the power, 2017-02-28 23:34:12]

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

[erikchen, 2017-03-01 00:05:18]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-03-01 00:05:22]

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

[commit-bot: I haz the power, 2017-03-01 00:06:27]

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

[erikchen, 2017-03-01 00:28:13]

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

[commit-bot: I haz the power, 2017-03-01 00:28:43]

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

[erikchen, 2017-03-01 00:34:41]

Description was changed from

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Update g_malloc_zones to be an array rather than a pointer.

BUG=693237
==========

[commit-bot: I haz the power, 2017-03-01 01:51:50]

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

[commit-bot: I haz the power, 2017-03-01 01:51:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-03-01 02:59:17]

Description was changed from

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Update g_malloc_zones to be an array rather than a pointer.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

[erikchen, 2017-03-01 02:59:46]

Description was changed from

==========
mac: Several minor fixes to allocator shim.

This CL includes:
  * Clean-up of tests to ensure that no state is carried between tests.
  * Update malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

[erikchen, 2017-03-01 02:59:52]

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

[commit-bot: I haz the power, 2017-03-01 03:00:27]

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-03-01 03:57:15]

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

[commit-bot: I haz the power, 2017-03-01 03:57:16]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-03-01 04:50:49]

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

[erikchen, 2017-03-01 04:50:50]

primiano, mark: Please review.

[Primiano Tucci (use gerrit), 2017-03-21 19:39:24]

Just have a doubt on the double-dep on allocator_shim.cc .
Everything else LGTM % some minor comments.

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

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:366: ChromeMallocZone* default_zone
=
can you add a comment explaining that the malloc zone is not listed in the
get_all_zones below?

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:368: if
(IsMallocZoneAlreadyStored(default_zone) &&
minor comment: you are now repeating this:
IsMallocZoneAlreadyStored(default_zone) && ...!= ...malloc in three places.
Maybe you could factor this, either as a flag of ReplaceZoneFunctions, or by
introducing a MaybeReplaceZoneFunction (or whatever other name)

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:411: memset(&new_functions, 0,
sizeof(MallocZoneFunctions));
Just MallocZoneFunctions new_functions = {} should be enough (and save a
precious line :P)?
same below

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_shim.cc:329: MallocZoneFunctions functions =
MallocZoneFunctionsToReplaceDefault();
out of curiosity why this doesn't happen in
allocator_shim_override_mac_symbols.h ?
I think with this we are back to allocator_shim depends on
allocator_interception_mac and viceversa, making IMHO harder to understand who
is the building block, and who builds on top of it (or maybe I am misfollowing
the code)?

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/malloc_z...
File base/allocator/malloc_zone_functions_mac.h (right):

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/malloc_z...
base/allocator/malloc_zone_functions_mac.h:42: struct MallocZoneFunctions {
can you add a static_assert(std::is_pod to this in some .cc file, so we are sure
we don't forget about all the discussion and accidentally regress this in the
future?

[Mark Mentovai, 2017-03-22 17:00:07]

LGTM, but a big “no” vote on binary patching system libraries, which you allude
to in the change description.

[erikchen, 2017-03-24 01:03:25]

Description was changed from

==========
mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Minor refactor in preparation for a mach_override based solution to
  intercept newly registered malloc zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

[erikchen, 2017-03-24 01:10:20]

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

[commit-bot: I haz the power, 2017-03-24 01:10:43]

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-03-24 01:27:55]

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

[commit-bot: I haz the power, 2017-03-24 01:27:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-03-24 21:28:57]

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

[commit-bot: I haz the power, 2017-03-24 21:29:25]

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

[erikchen, 2017-03-24 21:46:20]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-03-24 21:58:36]

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

[erikchen, 2017-03-24 21:59:03]

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

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:366: ChromeMallocZone* default_zone
=
On 2017/03/21 19:39:23, Primiano Tucci wrote:
> can you add a comment explaining that the malloc zone is not listed in the
> get_all_zones below?

Done.

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:368: if
(IsMallocZoneAlreadyStored(default_zone) &&
On 2017/03/21 19:39:23, Primiano Tucci wrote:
> minor comment: you are now repeating this:
> IsMallocZoneAlreadyStored(default_zone) && ...!= ...malloc in three places.
> Maybe you could factor this, either as a flag of ReplaceZoneFunctions, or by
> introducing a MaybeReplaceZoneFunction (or whatever other name)

Done.

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:411: memset(&new_functions, 0,
sizeof(MallocZoneFunctions));
On 2017/03/21 19:39:23, Primiano Tucci wrote:
> Just MallocZoneFunctions new_functions = {} should be enough (and save a
> precious line :P)?
> same below

Done.

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/allocato...
base/allocator/allocator_shim.cc:329: MallocZoneFunctions functions =
MallocZoneFunctionsToReplaceDefault();
On 2017/03/21 19:39:23, Primiano Tucci wrote:
> out of curiosity why this doesn't happen in
> allocator_shim_override_mac_symbols.h ?


> I think with this we are back to allocator_shim depends on
> allocator_interception_mac and viceversa, making IMHO harder to understand who
> is the building block, and who builds on top of it (or maybe I am misfollowing
> the code)?

allocator_shim depends on allocator_shim_default_dispatch_to_mac_zoned_malloc.
allocator_shim_default_dispatch_to_mac_zoned_malloc depends on allocator_shim.
This is already the case, and is because AllocatorDispatch is defined in
allocator_shim.

allocator_shim depends on allocator_interception_mac
allocator_interception_mac depends on malloc_zone_functions_mac

allocator_interception_mac.cc was unnecessarily including allocator_shim.h,
which might be what confused you.

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/malloc_z...
File base/allocator/malloc_zone_functions_mac.h (right):

https://codereview.chromium.org/2727463002/diff/80001/base/allocator/malloc_z...
base/allocator/malloc_zone_functions_mac.h:42: struct MallocZoneFunctions {
On 2017/03/21 19:39:23, Primiano Tucci wrote:
> can you add a static_assert(std::is_pod to this in some .cc file, so we are
sure
> we don't forget about all the discussion and accidentally regress this in the
> future?

Done. Added it right after
"""
MallocZoneFunctions g_malloc_zones[kMaxZoneCount];
"""

[commit-bot: I haz the power, 2017-03-24 21:59:04]

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

[erikchen, 2017-03-24 21:59:16]

On 2017/03/22 17:00:07, Mark Mentovai wrote:
> LGTM, but a big “no” vote on binary patching system libraries, which you
allude
> to in the change description.

Acknowledged, I updated the CL description.

[erikchen, 2017-03-24 21:59:20]

The CQ bit was unchecked by erikchen@chromium.org

[erikchen, 2017-03-24 21:59:21]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-03-24 21:59:21]

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/2727463002/#ps160001
(title: "more compile error.")

[commit-bot: I haz the power, 2017-03-24 22:00:01]

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-03-24 22:09:20]

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

[commit-bot: I haz the power, 2017-03-24 22:09:21]

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-03-27 21:19:07]

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

[erikchen, 2017-03-27 21:19:08]

sky: Please review chrome/

[sky, 2017-03-27 23:47:06]

LGTM

[erikchen, 2017-03-27 23:48:42]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:49:24]

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-03-28 01:11:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490658522825280,
"parent_rev": "b698a2cc70ef266c593b7515881c0dbf60106a17", "commit_rev":
"2f9bf057e1227bcee1d24e4b619894233946d26c"}

[commit-bot: I haz the power, 2017-03-28 01:12:12]

Description was changed from

==========
mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237
==========

to

==========
mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237

Review-Url: https://codereview.chromium.org/2727463002
Cr-Commit-Position: refs/heads/master@{#459964}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f9bf057e1227bcee1d24e4b6198...
==========

[commit-bot: I haz the power, 2017-03-28 01:12:13]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2f9bf057e1227bcee1d24e4b6198...