Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650363002:
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions. (Closed)
DescriptionRefactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Original-Commit-Position: refs/heads/master@{#446255}
Committed: https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3ac4d0590f6ff
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446440}
Committed: https://chromium.googlesource.com/chromium/src/+/d774b55241a0a1d4c7f0eba23592c4b5de4ce24c
Patch Set 1 #Patch Set 2 : rebase. #Patch Set 3 : Rebase. #
Total comments: 8
Patch Set 4 : Comments from primiano. #Patch Set 5 : Fix ASAN build error. #
Depends on Patchset: Messages
Total messages: 42 (33 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...
erikchen@chromium.org changed reviewers: + primiano@chromium.org
primiano: Please review.
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
mark: Please review. Original CL is https://codereview.chromium.org/2601573002/. primiano suggested that I do some refactoring, and also break into smaller CLs.
LGTM
The CQ bit was checked by erikchen@chromium.org
The CQ bit was unchecked by erikchen@chromium.org
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...
neat. LGTM with some comments/questons (mostly just me being paranoid). https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.h (right): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:43: MallocZoneFunctions* functions); maybe +const purely for documentation purposes https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (left): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:345: CHECK(!g_old_malloc && !g_old_calloc && !g_old_valloc && !g_old_realloc && shouldn't we keep these checks somewhere? https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:382: CHECK(g_old_malloc && g_old_calloc && g_old_valloc && g_old_free && same here? https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (right): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:300: functions->malloc = zone->malloc; not sure if it makes a difference, just checking: the old code was reading the values under the de-protection, which now happens only in the replace below. I guess the protection is only RO -> RW, and you can always read-back them?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.h (right): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:43: MallocZoneFunctions* functions); On 2017/01/26 01:45:14, Primiano Tucci wrote: > maybe +const purely for documentation purposes Done. https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (left): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:345: CHECK(!g_old_malloc && !g_old_calloc && !g_old_valloc && !g_old_realloc && On 2017/01/26 01:45:14, Primiano Tucci wrote: > shouldn't we keep these checks somewhere? These checks are pointless, since these values are nullptr initialized, see struct MallocZoneFunctions. https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:382: CHECK(g_old_malloc && g_old_calloc && g_old_valloc && g_old_free && On 2017/01/26 01:45:14, Primiano Tucci wrote: > same here? There's more value to keeping these checks. I've re-added them in ReplaceZoneFunctions. https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (right): https://codereview.chromium.org/2650363002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:300: functions->malloc = zone->malloc; On 2017/01/26 01:45:14, Primiano Tucci wrote: > not sure if it makes a difference, just checking: the old code was reading the > values under the de-protection, which now happens only in the replace below. > I guess the protection is only RO -> RW, and you can always read-back them? correct.
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by erikchen@chromium.org
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/2650363002/#ps60001 (title: "Comments from primiano.")
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": 60001, "attempt_start_ts": 1485412201580400,
"parent_rev": "756523826c9e6498a4d4e9998f83b962375a30bc", "commit_rev":
"50a11072abe7c112ed23956929b3ac4d0590f6ff"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
==========
to
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446255}
Committed:
https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
Message was sent while issue was closed.
Description was changed from
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446255}
Committed:
https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
==========
to
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446255}
Committed:
https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
==========
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
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/2650363002/#ps80001 (title: "Fix ASAN build error.")
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": 80001, "attempt_start_ts": 1485463727540040,
"parent_rev": "f86538c24cd36e87111726eabfba4f2780fc69d8", "commit_rev":
"d774b55241a0a1d4c7f0eba23592c4b5de4ce24c"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446255}
Committed:
https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
==========
to
==========
Refactor allocator_interception_mac to use {Store,Replace}ZoneFunctions.
This CL has no intended behavior change.
Pulling out the logic for {Store,Replace}ZoneFunctions allows the allocator shim
to easily integrate with the existing malloc interception code.
BUG=665567
Review-Url: https://codereview.chromium.org/2650363002
Cr-Original-Commit-Position: refs/heads/master@{#446255}
Committed:
https://chromium.googlesource.com/chromium/src/+/50a11072abe7c112ed23956929b3...
Review-Url: https://codereview.chromium.org/2650363002
Cr-Commit-Position: refs/heads/master@{#446440}
Committed:
https://chromium.googlesource.com/chromium/src/+/d774b55241a0a1d4c7f0eba23592...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d774b55241a0a1d4c7f0eba23592... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
