Chromium Code Reviews Chromium Code Reviews
Issue
2706453004:
Revert of base: make CHECK macros trap at distinct addresses in official builds (Closed)
DescriptionRevert of base: make CHECK macros trap at distinct addresses in official builds (patchset #10 id:180001 of https://codereview.chromium.org/2502953003/ )
Reason for revert:
Broke compile https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20ChromeOS/builds/29509
Original issue's 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
TBR=thakis@chromium.org,torne@chromium.org,brettw@chromium.org,danakj@chromium.org,mark@chromium.org,primiano@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=664209, 599867
Review-Url: https://codereview.chromium.org/2706453004
Cr-Commit-Position: refs/heads/master@{#451384}
Committed: https://chromium.googlesource.com/chromium/src/+/4c98a8c2c794b10f70fc22e02128139252a0b0c1
Patch Set 1 #
Messages
Total messages: 6 (3 generated)
|
||||||||||||||||||||||||||||
