Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(56)

Issue 1577883002: Add run-time CHECK to smoke-test allocator overrides (Closed)

Created:
3 years, 3 months ago by Primiano Tucci (use gerrit)
Modified:
3 years, 3 months ago
CC:
chromium-reviews, wfh+watch_chromium.org, grt+watch_chromium.org, Dai Mikurube (NOT FULLTIME), vmpstr+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add run-time CHECK to smoke-test allocator overrides As part of the refactoring work on the allocator, it emerged that it would be nice to have a test that ensures that we don't accidentally break things along the way and not detected that. This CL adds a runtime CHECK() that verifies that: - On Windows (non-component build) the shim layer has been initialized. Concretely this checks that chrome called the shimmed _heap_init() method and not the one from libcmt. - On Linux desktop, the malloc symbols are being overridden by tcmalloc (only when USE_TCMALLOC is defined). The rationale of this test is: if _heap_init was shimmed there are very good chances that malloc (& friends) were shimmed as well. Likewise on Linux for mallopt() <-> malloc(). BUG=564618 Committed: https://crrev.com/be9d6fce01061afde845fd7aa2335e5757d6a421 Cr-Commit-Position: refs/heads/master@{#369007}

Patch Set 1 #

Patch Set 2 : add also tcmalloc check #

Patch Set 3 : Mac is too cool to have malloc.h in the same place. Meh #

Total comments: 3

Patch Set 4 : use ALLOCATOR_SHIM #

Patch Set 5 : mallopt crazyiness #

Patch Set 6 : fix comment on mallopt #

Patch Set 7 : Rebase #

Patch Set 8 : AAAAArgh, inconsistent trailing dot in README.chromium #

Unified diffs Side-by-side diffs Delta from patch set Stats (+91 lines, -4 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 2 chunks +3 lines, -0 lines 0 comments Download
A base/allocator/allocator_check.h View 1 1 chunk +18 lines, -0 lines 0 comments Download
A base/allocator/allocator_check.cc View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
M base/allocator/allocator_shim_win.cc View 1 2 chunks +7 lines, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/app/content_main_runner.cc View 1 2 chunks +7 lines, -0 lines 0 comments Download
M third_party/tcmalloc/README.chromium View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h View 1 2 3 4 5 3 chunks +8 lines, -3 lines 0 comments Download
M third_party/tcmalloc/chromium/src/tcmalloc.cc View 1 2 3 4 1 chunk +7 lines, -1 line 0 comments Download

Messages

Total messages: 32 (15 generated)
Primiano Tucci (use gerrit)
Will, I tried to give a shoot at the test issue. I first tried with ...
3 years, 3 months ago (2016-01-11 16:42:11 UTC) #3
Primiano Tucci (use gerrit)
following +Nico suggestion (in a previous offline chat), adding also a check to make sure ...
3 years, 3 months ago (2016-01-11 18:45:12 UTC) #6
Will Harris
https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc File base/allocator/allocator_check.cc (right): https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc#newcode23 base/allocator/allocator_check.cc:23: #if defined(OS_WIN) && !defined(COMPONENT_BUILD) I think this needs a ...
3 years, 3 months ago (2016-01-11 22:51:31 UTC) #7
Primiano Tucci (use gerrit)
https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc File base/allocator/allocator_check.cc (right): https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc#newcode23 base/allocator/allocator_check.cc:23: #if defined(OS_WIN) && !defined(COMPONENT_BUILD) On 2016/01/11 22:51:31, Will Harris ...
3 years, 3 months ago (2016-01-11 23:32:05 UTC) #8
Primiano Tucci (use gerrit)
https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc File base/allocator/allocator_check.cc (right): https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocator_check.cc#newcode23 base/allocator/allocator_check.cc:23: #if defined(OS_WIN) && !defined(COMPONENT_BUILD) On 2016/01/11 22:51:31, Will Harris ...
3 years, 3 months ago (2016-01-12 11:36:02 UTC) #9
Will Harris
please can you add a test that calls base::allocator::IsAllocatorInitialized() somewhere in base_unittests - we'd want ...
3 years, 3 months ago (2016-01-12 15:08:38 UTC) #10
Primiano Tucci (use gerrit)
On 2016/01/12 15:08:38, Will Harris (in DC) wrote: > please can you add a test ...
3 years, 3 months ago (2016-01-12 16:06:36 UTC) #11
Will Harris
On 2016/01/12 16:06:36, Primiano Tucci wrote: > On 2016/01/12 15:08:38, Will Harris (in DC) wrote: ...
3 years, 3 months ago (2016-01-12 16:22:40 UTC) #12
Primiano Tucci (use gerrit)
+sievers for +2 lines in content/app/content_main_runner.cc +thakis for base/ build file changes
3 years, 3 months ago (2016-01-12 18:44:26 UTC) #14
no sievers
On 2016/01/12 18:44:26, Primiano Tucci wrote: > +sievers for +2 lines in content/app/content_main_runner.cc lgtm > ...
3 years, 3 months ago (2016-01-12 18:47:27 UTC) #15
Nico
lgtm
3 years, 3 months ago (2016-01-12 20:50:27 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577883002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577883002/100001
3 years, 3 months ago (2016-01-12 20:52:18 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/163913)
3 years, 3 months ago (2016-01-12 20:57:38 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577883002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577883002/120001
3 years, 3 months ago (2016-01-12 21:03:12 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1577883002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1577883002/150001
3 years, 3 months ago (2016-01-12 21:06:38 UTC) #28
commit-bot: I haz the power
Committed patchset #8 (id:150001)
3 years, 3 months ago (2016-01-12 22:16:19 UTC) #30
commit-bot: I haz the power
3 years, 3 months ago (2016-01-12 22:18:16 UTC) #32
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/be9d6fce01061afde845fd7aa2335e5757d6a421
Cr-Commit-Position: refs/heads/master@{#369007}

Powered by Google App Engine
This is Rietveld 408576698