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

Issue 2502953003: base: make CHECK macros trap at distinct addresses in official builds (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 months, 1 week ago by Primiano Tucci (use gerrit)
Modified:
8 months ago
CC:
chromium-reviews, danakj, brettw, esprehn, tkent, Mark Mentovai, scottmg
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

base: make CHECK macros trap at distinct addresses in official builds Abstract -------- CHECK() is the macro used all over the places (~4K occurrences without counting for dupes due to inlining) for release-time assertions. It is enabled in a minimal form (crash without a message) in official builds. It needs to be fast as it is used in lot of fastpath. It needs to not emit too much code, as it is used in lot of places. It needs to guarantee that crash reports can pinpoint to the right location when hitting a CHECK. - Back in the days CHECK was fat, slow, but crash-friendly. - After crrev.com/1982123002 it became tiny, fast but not crash friendly. - This CL is making it a bit less tiny (+28/+128K), fast and crash friendly. The problem this CL deals with is the case of multiple CHECK()s within the same function. Also, adds a test that covers Mac, Linux and Android. A bit of history: ----------------- Before crrev.com/1982123002 (reverted and later re-landed in crrev.com/2125923002) CHECK() in official builds was essentially: if (!condition) BreakDebugger() It was later found that this approach was not efficient, both in terms of binary size and performance. More importantly it was a regression w.r.t. blink's assert that were later switched to CHECK(). "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation" The major reason for this is DebuggerBreak being quite complex and not treated as noreturn by the compiler. It seems (see crbug.com/664209 for more) that the most efficient way to handle these checks is ending up with: Source code: CHECK(cond1); CHECK(cond2); ... Ideal assembly: (ideal for perf and binary, but not for crash reports, more below) compare_opcode cond1; jump_if_zero prologue; compare_opcode cond2; jump_if_zero prologue; ... prologue: trap_instruction(s) Rather than something like: compare_opcode cond1; jump_if_NOT_zero next1; trap_instruction(s) next1: compare_opcode cond2; jump_if_NOT_zero next2; trap_instruction(s) next2: ... Where essentially the trap instructions are interleaved within the main execution flow. This is even worse if the trap instruction is actually a call to a function, with annex frame initialization, as in the case of BreakDebugger(). That bloats the binary and reduces i-cache hotness for the main flow. crrev.com/1982123002 recently fixed the situation making the assembly look like the ideal case above. Unfortunately this caused another problem due the extreme optimization: once the program crashes in "trap_instruction(s)", there is no easy way to tell which condition caused the trap. In practice this translates into the inability of tell which CHECK failed in a function that has more than one check. This CL: -------- Re-addresses crrev.com/2125923002, adding an extra instruction after the trap which creates an opcode with a unique counter. This prevents the compiler from folding the trap instructions, still applying no-return optimizations. Also by doing this the various prologue get properly attributed to the CHECK line in debugging symbols. Binary size inflation on official builds: ----------------------------------------- Android arm: 48684276 -> 48712948 = 28 K Android arm64: 85611800 -> 85665048 = 53 K Android x86_64: 91904944 -> 91933616 = 28 K Linux x86_64: 124219488 -> 124346464 = 124 K (Android build with -Os, hence why the difference between the two) BUG=664209, 599867 Review-Url: https://codereview.chromium.org/2502953003 Cr-Commit-Position: refs/heads/master@{#451381} Committed: https://chromium.googlesource.com/chromium/src/+/8c972d0e190168b4b5621e81563f319563fd0af8

Patch Set 1 #

Patch Set 2 : . #

Total comments: 1

Patch Set 3 : use inline asm #

Patch Set 4 : fix comments #

Total comments: 1

Patch Set 5 : . #

Patch Set 6 : . #

Patch Set 7 : . #

Total comments: 2

Patch Set 8 : Fix tests (an uncountable amount of tables have been flipped for this) #

Total comments: 9

Patch Set 9 : Address Mark's comment + fixtestsforrealz #

Total comments: 2

Patch Set 10 : use static_cast + primitive type #

Messages

Total messages: 85 (51 generated)
Primiano Tucci (use gerrit)
CHECK() is back! CL description should have all the details. I didn't manage to run ...
11 months ago (2016-11-21 16:46:23 UTC) #11
Primiano Tucci (use gerrit)
+torne
11 months ago (2016-11-21 16:46:52 UTC) #13
Primiano Tucci (use gerrit)
Had a chat offline with torne, he is suggesting that clobbering one register during the ...
11 months ago (2016-11-21 17:18:18 UTC) #14
Primiano Tucci (use gerrit)
+tkent FYI as per discussion in "[blink-dev] Update of wtf/Assertions.h, and ASSERT macros deprecation"
11 months ago (2016-11-22 14:58:15 UTC) #17
Primiano Tucci (use gerrit)
Brett / Dana: opinions here (see #14) ?
11 months ago (2016-11-23 18:24:15 UTC) #20
danakj
On Wed, Nov 23, 2016 at 10:24 AM, <primiano@chromium.org> wrote: > Brett / Dana: opinions ...
11 months ago (2016-11-23 22:22:22 UTC) #21
Nico
Thanks for the detailed writeup. I think not clobbering registers is helpful if we can ...
10 months, 3 weeks ago (2016-12-02 02:04:03 UTC) #23
Torne
On 2016/12/02 02:04:03, Nico wrote: > Thanks for the detailed writeup. I think not clobbering ...
10 months, 3 weeks ago (2016-12-02 12:37:17 UTC) #24
Primiano Tucci (use gerrit)
All right I think then the best approach is to just manually put the various ...
10 months, 3 weeks ago (2016-12-02 15:08:28 UTC) #25
Nico
Maybe we should just call abort(). That's marked noreturn, takes 0 arguments, and is standard. ...
10 months, 3 weeks ago (2016-12-02 15:29:25 UTC) #26
Primiano Tucci (use gerrit)
On 2016/12/02 15:29:25, Nico wrote: > Maybe we should just call abort(). That's marked noreturn, ...
10 months, 3 weeks ago (2016-12-02 16:47:54 UTC) #27
Torne
Yeah. The problem there is that x86 doesn't have conditional calls, and so if the ...
10 months, 2 weeks ago (2016-12-05 12:37:06 UTC) #28
Nico
Ok, the per-arch ud2s sound like a reasonable way to go to me.
10 months, 2 weeks ago (2016-12-05 21:08:46 UTC) #29
Primiano Tucci (use gerrit)
On 2016/12/05 21:08:46, Nico wrote: > Ok, the per-arch ud2s sound like a reasonable way ...
8 months, 1 week ago (2017-02-13 15:24:11 UTC) #36
Mark Mentovai
https://codereview.chromium.org/2502953003/diff/60001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2502953003/diff/60001/base/logging.h#newcode484 base/logging.h:484: // either a SIGTRAP or a SIGABRT depending on ...
8 months, 1 week ago (2017-02-13 16:22:09 UTC) #37
Torne
On 2017/02/13 15:24:11, Primiano Tucci wrote: > On 2016/12/05 21:08:46, Nico wrote: > > Ok, ...
8 months, 1 week ago (2017-02-13 17:10:05 UTC) #38
Primiano Tucci (use gerrit)
PTAL I think I finally have a version that keep all the constraints satisfied: - ...
8 months ago (2017-02-16 21:38:45 UTC) #50
Primiano Tucci (use gerrit)
+scottmg FYI
8 months ago (2017-02-16 21:43:52 UTC) #51
Torne
Looks pretty reasonable. I'm mildly scared by using hlt on arm64 but I guess entering ...
8 months ago (2017-02-17 11:06:31 UTC) #54
Torne
Also, the CL description is no longer quite accurate as it still mentions putting line ...
8 months ago (2017-02-17 11:08:01 UTC) #55
Primiano Tucci (use gerrit)
> I'm mildly scared by using hlt on arm64 but I guess entering debug state ...
8 months ago (2017-02-17 12:37:35 UTC) #59
Mark Mentovai
This is lovely! https://codereview.chromium.org/2502953003/diff/140001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2502953003/diff/140001/base/logging.h#newcode525 base/logging.h:525: asm volatile("int $3; push %0; ud2;" ...
8 months ago (2017-02-17 14:24:28 UTC) #63
Primiano Tucci (use gerrit)
> This is lovely! Not quite the same reaction I had when I hit the ...
8 months ago (2017-02-17 14:51:07 UTC) #66
Mark Mentovai
LGTM https://codereview.chromium.org/2502953003/diff/140001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2502953003/diff/140001/base/logging.h#newcode525 base/logging.h:525: asm volatile("int $3; push %0; ud2;" ::"i"(__COUNTER__ % ...
8 months ago (2017-02-17 15:04:33 UTC) #67
Primiano Tucci (use gerrit)
https://codereview.chromium.org/2502953003/diff/160001/base/logging.h File base/logging.h (right): https://codereview.chromium.org/2502953003/diff/160001/base/logging.h#newcode525 base/logging.h:525: asm volatile("int3; ud2; push %0;" ::"i"((uint8_t)__COUNTER__)) On 2017/02/17 15:04:33, ...
8 months ago (2017-02-17 15:08:47 UTC) #70
Mark Mentovai
superlgtm
8 months ago (2017-02-17 15:14:37 UTC) #71
Primiano Tucci (use gerrit)
+wfh: I thought I added you various patchsets ago but that just realized that was ...
8 months ago (2017-02-17 15:17:17 UTC) #72
Primiano Tucci (use gerrit)
May the waterfall and all the hidden devices/bots/configuration have mercy of this CL. (I have ...
8 months ago (2017-02-17 18:28:14 UTC) #73
Will Harris
as long as this doesn't regress future CL https://codereview.chromium.org/2697423002/ I'm happy with this :)
8 months ago (2017-02-17 18:46:47 UTC) #74
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2502953003/180001
8 months ago (2017-02-17 19:48:07 UTC) #79
commit-bot: I haz the power
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/8c972d0e190168b4b5621e81563f319563fd0af8
8 months ago (2017-02-17 21:09:42 UTC) #82
alph
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/2706453004/ by alph@chromium.org. ...
8 months ago (2017-02-17 21:27:44 UTC) #83
Primiano Tucci (use gerrit)
On 2017/02/17 21:27:44, alph wrote: > A revert of this CL (patchset #10 id:180001) has ...
8 months ago (2017-02-17 22:30:41 UTC) #84
Primiano Tucci (use gerrit)
8 months ago (2017-02-21 11:05:06 UTC) #85
Message was sent while issue was closed.
This is relanding in https://codereview.chromium.org/2705053002/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa