Update of wtf/Assertions.h, and ASSERT macros deprecation

[TAMURA, Kent]

Please replace the following macros in your code, except wtf/Vector.h and wtf/HashTable.h.

ASSERT  ->  DCHECK or DCHECK_EQ/NE/LE/LT/GE/GT

RELEASE_ASSERT  ->  CHECK or CHECK_EQ/NE/LE/LT/GE/GT

ENABLE(ASSERT)  ->  DCHECK_IS_ON()

ASSERT_NO_REACHED()  ->  NOTREACHED()

ASSERT_UNUSED  ->  DCHECK* and ALLOW_UNUSED_LOCAL() if necessary.

ASSERT_WITH_SECURITY_IMPLICATION  ->  SECURITY_DCHECK

Also, please remove WTF_LOG usage, use VLOG() or DVLOG instead.

You can associate cleanup CLs for them to crbug.com/596760.

----------------------------------------------------------------------------------------

Changes we already finished:

• RELEASE_ASSERT_WITH_SECURITY_IMPLICATION was already replaced with SECURITY_CHECK.
• RELEASE_ASSERT_NOT_REACHED was renamed to RELEASE_NOTREACHED.
• ASSERT_ARG was removed.  Please use DCHECK.
• BACKTRACE() was removed.  Please use WTFReportBacktrace() or base::debug::StackTrace.
  StackTrace has a downside; crbug.com/598886
• FATAL() was removed.   Please use DLOG(FATAL).
• WTF_LOG_ERROR() was removed.  Please use DLOG(ERROR).
• ASSERT_WITH_MESSAGE and RELEASE_ASSERT_WITH_MESSAGE were removed.  We can specify messages to DCHECK() and CHECK().
• platform/NotImplemented.* were removed in order to remove WTFLogVerbose() defined in Assertions.h.  Please use NOTIMPLEMENTED().

FAQ:

* Why don't we update wtf/Vector.h and wtf/HashTable.h?

Chromecast team uses Release + DCHECK_ALWAYS_ON + ENABLE_ASSERT=0  configuration daily because of device memory restriction.  Enabling ASSERTs in Blink increases the binary size significantly, and the major part of the increase comes from wtf/Vector.h and wtf/HashTable.h.  So we'd like to keep a way to disable assertions in these files.

--

TAMURA Kent
Software Engineer, Google

[Peter Kasting]

On Thu, Mar 31, 2016 at 12:55 AM, TAMURA, Kent <tk...@chromium.org> wrote:

ASSERT_WITH_SECURITY_IMPLICATION  ->  SECURITY_DCHECK

What's that?  In Chromium code we say that any assertion with security implications should be a CHECK.

PK

[Elliott Sprehn]

Some security asserts are too slow. For example having an extra virtual call inside every type cast function. The security asserts are used by clusterfuzz and friends.

[Yuta Kitamura]

On Thu, Mar 31, 2016 at 4:55 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Please replace the following macros in your code, except wtf/Vector.h and wtf/HashTable.h.

ASSERT  ->  DCHECK or DCHECK_EQ/NE/LE/LT/GE/GT

RELEASE_ASSERT  ->  CHECK or CHECK_EQ/NE/LE/LT/GE/GT

ENABLE(ASSERT)  ->  DCHECK_IS_ON()

ASSERT_NO_REACHED()  ->  NOTREACHED()

ASSERT_UNUSED  ->  DCHECK* and ALLOW_UNUSED_LOCAL() if necessary.

ASSERT_WITH_SECURITY_IMPLICATION  ->  SECURITY_DCHECK

Also, please remove WTF_LOG usage, use VLOG() or DVLOG instead.

I think this is well known in Chromium, but some Blink devs may not be aware so let me stress: Please prefer DVLOG() over VLOG(). VLOG() puts your logging code into Release binaries, which is not desirable at our scale.

[Hayato Ito]

It looks we have to guard DCHECK(EXP) by "DCHECK_IS_ON()" if EXP uses what does not *exist* in a release build.

e.g.

Before:

     ASSERT(!EventDispatchForbiddenScope::isEventDispatchForbidden());

After:

#if DCHECK_IS_ON()

     DCHECK(!EventDispatchForbiddenScope::isEventDispatchForbidden());

#endif

Without the guard, the compile fails in a release build. Is this an intentional behavior?

[Dana Jansens]

On Tue, Apr 5, 2016 at 6:34 AM, Hayato Ito <hay...@chromium.org> wrote:

It looks we have to guard DCHECK(EXP) by "DCHECK_IS_ON()" if EXP uses what does not *exist* in a release build.

e.g.

Before:

     ASSERT(!EventDispatchForbiddenScope::isEventDispatchForbidden());

After:

#if DCHECK_IS_ON()

     DCHECK(!EventDispatchForbiddenScope::isEventDispatchForbidden());

#endif

Without the guard, the compile fails in a release build. Is this an intentional behavior?

DCHECK uses the things passed to it even if they are disabled, to avoid warnings about unused variables.

[Takayoshi Kochi]

On Wed, Apr 6, 2016 at 3:04 AM, Dana Jansens <dan...@chromium.org> wrote:

On Tue, Apr 5, 2016 at 6:34 AM, Hayato Ito <hay...@chromium.org> wrote:

It looks we have to guard DCHECK(EXP) by "DCHECK_IS_ON()" if EXP uses what does not *exist* in a release build.

e.g.

Before:

     ASSERT(!EventDispatchForbiddenScope::isEventDispatchForbidden());

After:

#if DCHECK_IS_ON()

     DCHECK(!EventDispatchForbiddenScope::isEventDispatchForbidden());

#endif

Without the guard, the compile fails in a release build. Is this an intentional behavior?

DCHECK uses the things passed to it even if they are disabled, to avoid warnings about unused variables.

 

To add a few words,

the case above happened because the whole EventDispatchForbiddenScope is conditionally

compiled in #ifdef ENABLE(ASSERT), it was necessary.

In general you do not have to use DCHECK_IS_ON() around DCHECK converted from ASSERT(),

but may have to (case-by-case basis).

--

Takayoshi Kochi

[Hayato Ito]

Thanks. That makes sense. DCHECK is not completely equivalent to ASSERT in this sense.

[TAMURA, Kent]

On Thu, Mar 31, 2016 at 4:55 PM, TAMURA, Kent <tk...@chromium.org> wrote:

Please replace the following macros in your code, except wtf/Vector.h and wtf/HashTable.h.

ASSERT  ->  DCHECK or DCHECK_EQ/NE/LE/LT/GE/GT

RELEASE_ASSERT  ->  CHECK or CHECK_EQ/NE/LE/LT/GE/GT

We found CHECK was significantly slower than RELEASE_ASSERT, and a replacement CL caused a performance regression.  See crbug.com/599867.

Please do not replace RELEASE_ASSERT yet.

[Kentaro Hara]

Do you have any update about the performance regression caused by the RELEASE_ASSERT => CHECK replacement?

(Since people started using CHECK/DCHECK macros, it would be better if we could fix the regression sooner.)

--

Kentaro Hara, Tokyo, Japan

[Dana Jansens]

On Fri, Apr 29, 2016 at 2:48 AM, Kentaro Hara <har...@chromium.org> wrote:

Do you have any update about the performance regression caused by the RELEASE_ASSERT => CHECK replacement?

(Since people started using CHECK/DCHECK macros, it would be better if we could fix the regression sooner.)

Android folks have not replied to multiple pings on email threads. I propose we revert the android-specific behaviour in check to prevent the perf regression.

[Dana Jansens]

On Fri, Apr 29, 2016 at 2:48 AM, Kentaro Hara <har...@chromium.org> wrote:

Do you have any update about the performance regression caused by the RELEASE_ASSERT => CHECK replacement?

I think we can try this again now. https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c26

[Elliott Sprehn]

This is not ready yet, it needs to be as fast as wtf was for Release builds that are not official builds. Very few people build official builds and few documents even tell you to do it. We can't switch to base things that are slower except in official builds.

[Dana Jansens]

On Wed, May 4, 2016 at 1:19 PM, Elliott Sprehn <esp...@chromium.org> wrote:

This is not ready yet, it needs to be as fast as wtf was for Release builds that are not official builds. Very few people build official builds and few documents even tell you to do it. We can't switch to base things that are slower except in official builds.

Non-official release builds are not meant to have the same level of performance. Perf bots and anyone doing perf analysis should be using an official build. To do so, add this to your GN args:

is_official_build = true
is_chrome_branded = true

You can find these on the instructions to set up your build: https://www.chromium.org/developers/gn-build-configuration  Is there some other documentation you'd like us to fix?

This sounds orthogonal to using CHECK vs RELEASE_ASSERT. If anyone has been doing performance measurements in a non-official build until now unfortunately their numbers aren't very accurate. We use different compiler settings for non-official builds as well. Perhaps we should send a PSA of sorts of people are not generally aware they need to build official when doing perf measurements?

[Brett Wilson]

On Wed, May 4, 2016 at 1:29 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, May 4, 2016 at 1:19 PM, Elliott Sprehn <esp...@chromium.org> wrote:

This is not ready yet, it needs to be as fast as wtf was for Release builds that are not official builds. Very few people build official builds and few documents even tell you to do it. We can't switch to base things that are slower except in official builds.

Non-official release builds are not meant to have the same level of performance. Perf bots and anyone doing perf analysis should be using an official build. To do so, add this to your GN args:

is_official_build = true
is_chrome_branded = true

You can find these on the instructions to set up your build: https://www.chromium.org/developers/gn-build-configuration  Is there some other documentation you'd like us to fix?

This sounds orthogonal to using CHECK vs RELEASE_ASSERT. If anyone has been doing performance measurements in a non-official build until now unfortunately their numbers aren't very accurate. We use different compiler settings for non-official builds as well. Perhaps we should send a PSA of sorts of people are not generally aware they need to build official when doing perf measurements?

Yes, exactly. It doesn't make sense to say that Release must have X level of performance because that's not what Release is for. This is unfortunately confusing.

There are three optimization levels, Debug, Release, and Official. Many people confuse "official" and "Chrome-branded." The fact that Official is a separate flag I suspect is an artifact of how the Visual Studio build used to be set up. Don't even ask what happens if you set the official flag on a debug build.

If it was up to me, we would only have two. But some people like the status quo because they want a compromise between fast-to-execute and fast-to-build. It could be that such people might be happy with a flag to toggle link-time-code-generation (the most significant difference between these builds).

And in fact if you ask what Release is, the only definition is really "most optimizations except the really slow ones." Doing benchmarks on such a build is likely to be a waste of time.

Brett

[Elliott Sprehn]

On Wed, May 4, 2016 at 1:29 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, May 4, 2016 at 1:19 PM, Elliott Sprehn <esp...@chromium.org> wrote:

This is not ready yet, it needs to be as fast as wtf was for Release builds that are not official builds. Very few people build official builds and few documents even tell you to do it. We can't switch to base things that are slower except in official builds.

Non-official release builds are not meant to have the same level of performance. Perf bots and anyone doing perf analysis should be using an official build. To do so, add this to your GN args:

is_official_build = true
is_chrome_branded = true

You can find these on the instructions to set up your build: https://www.chromium.org/developers/gn-build-configuration  Is there some other documentation you'd like us to fix?

This sounds orthogonal to using CHECK vs RELEASE_ASSERT. If anyone has been doing performance measurements in a non-official build until now unfortunately their numbers aren't very accurate. We use different compiler settings for non-official builds as well. Perhaps we should send a PSA of sorts of people are not generally aware they need to build official when doing perf measurements?

Those people are me, and everyone else doing perf measurement for years on our team. Having to do a full recompile with official turned on is not acceptable. We need to make base fast by default, not just when you set some magic flags.

- E 

[Brett Wilson]

But official is slow to build because it does more optimization!

Brett

[Elliott Sprehn]

On Wed, May 4, 2016 at 1:42 PM, Brett Wilson <bre...@chromium.org> wrote:

On Wed, May 4, 2016 at 1:29 PM, Dana Jansens <dan...@chromium.org> wrote:

...

Yes, exactly. It doesn't make sense to say that Release must have X level of performance because that's not what Release is for. This is unfortunately confusing.

There are three optimization levels, Debug, Release, and Official. Many people confuse "official" and "Chrome-branded." The fact that Official is a separate flag I suspect is an artifact of how the Visual Studio build used to be set up. Don't even ask what happens if you set the official flag on a debug build.

If it was up to me, we would only have two. But some people like the status quo because they want a compromise between fast-to-execute and fast-to-build. It could be that such people might be happy with a flag to toggle link-time-code-generation (the most significant difference between these builds).

And in fact if you ask what Release is, the only definition is really "most optimizations except the really slow ones." Doing benchmarks on such a build is likely to be a waste of time.

That's extremely insulting, we've been optimizing performance on Blink for years very successfully, all in release builds and not in official ones. If base isn't going to be fast we just won't use it. We should revert to RELEASE_ASSERT for all blink, I see a bunch of CHECKS were added in stuff that some people said were not "performance critical", but all of blink is performance critical. If CHECK is slow in some configurations we shouldn't use it.

- E 

[Brett Wilson]

It sounds like my proposal to make Release==Official with a flag for LTCG would solve your problem.

Brett 

[Scott Graham]

I think it's fair to say that Release is *representative* of Official performance. So it's fine to do lightweight performance comparisons between two alternative implementations and guess that it won't be too misleading. This shouldn't really change if some logging macros get a bit slower or faster though.

Absolute benchmark performance especially in Blink, V8, and allocator-sensitive code has always been very different between Release and Official though.

It seems like the underlying problem in 599867 is that Android shipped logs in CHECK for a long time, which seems crazy, but that's fixed now.

[Brett Wilson]

On Wednesday, May 4, 2016, Scott Graham <sco...@chromium.org> wrote:

Isn't that what we already have? The flag is named buildtype=Dev/Official or is_official_build=false/true.

In GYP on Windows, Official == /Os and Release == /O2 so that makes it really exciting.

In GN Official is really close to being Release + LTCG (or maybe it actually is, somebody will have to check) because I cleaned it up. But with the current naming it will diverge again and I'm not comfortable recommending people make assumptions on it.

The only difference I can see in particular in Chrome logging between Official and Release is that Official doesn't print stack traces or the actual log message to save binary space. This actually seems like a good difference and perhaps I've argued myself out of my previous "there should be only two build variants" opinion: most bots run Release which makes the tests run faster, but we definitely want the backtraces and messages when stuff fails on the bots. For distribution, these take extra space and aren't very useful.

Since the differences here are so small, it shouldn't make a performance difference for most benchmarks. If your benchmark is sensitive to the extra code injected to print the back trace, it will also be sensitive to LTCG and you should really be testing those benchmarks in Official anyway.

Brett

[Tom Hudson]

I have no intent to insult, but when we were trying to get Chrome on Android performance adequate to ship, we never got valid benchmark or optimization results from Release builds; any work I did where I forgot to wait for an official build was wasted. So it may partially be a function of target platform, but I think Brett's statement was factual.

Tom

[Hayato Ito]

I got a performance regression report in https://bugs.chromium.org/p/chromium/issues/detail?id=602318.

Have you ever heard a similar performance regression by *D*CHECK?

[Kentaro Hara]

As far as I see the performance graph, I'd say the regression is a noise (in particular, chromium-rel-win7- tends to be very flaky).

Given that chromium-rel-* don't enable DCHECK, the ASSERT => DCHECK replacement shouldn't affect the performance.

[Ivan Kotenkov]

There were desktop regressions too (see https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c19 and https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c21), I didn't have time to investigate them and I think they still exist.

[Dana Jansens]

On Tue, May 10, 2016 at 8:20 AM, Ivan Kotenkov <kote...@yandex-team.ru> wrote:

There were desktop regressions too (see https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c19 and https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c21), I didn't have time to investigate them and I think they still exist.

Oh I think I confused this with the bug around TLS storage. I do see things get better on windows for that test with the revert, so there must be some codegen difference between CHECK and RELEASE_ASSERT still on official builds.

[Brett Wilson]

On Tue, May 10, 2016 at 1:42 PM, Dana Jansens <dan...@chromium.org> wrote:

On Tue, May 10, 2016 at 8:20 AM, Ivan Kotenkov <kote...@yandex-team.ru> wrote:

There were desktop regressions too (see https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c19 and https://bugs.chromium.org/p/chromium/issues/detail?id=599867#c21), I didn't have time to investigate them and I think they still exist.

Oh I think I confused this with the bug around TLS storage. I do see things get better on windows for that test with the revert, so there must be some codegen difference between CHECK and RELEASE_ASSERT still on official builds.

Interesting, any way we can optimize this sounds like it will be important.

Brett

[Brett Wilson]

I had a 32-bit Windows release build handy.

  CHECK(debug::EnableInProcessStackDumping());

0152B0CB E8 60 70 CB FF       call        base::debug::EnableInProcessStackDumping (011E2130h)  

0152B0D0 84 C0                test        al,al  

0152B0D2 75 05                jne         base::TestSuite::Initialize+0F0h (0152B0D9h)  

0152B0D4 E8 F7 5D CA FF       call        base::debug::BreakDebugger (011D0ED0h)

  RELEASE_ASSERT(!s_initialized);

00C951F4 80 3D 6C 88 E0 00 00 cmp         byte ptr ds:[0E0886Ch],0  

00C951FB 74 07                je          WTF::initialize+13h (0C95204h)  

00C951FD C6 05 00 00 00 00 00 mov         byte ptr ds:[0],0  

These are different because CHECK is calling a function and testing the return value in "al", and the RELEASE_ASSERT one I picked is just checking a global.

Both expand to a test (whatever is specified), and a two-byte conditional jump that's taken when the assertion passes. CHECK does a function call but RELEASE_ASSERT does a write to null. Looking at the code bytes (these are normally not shown by Visual Studio), I was surprised the function call is fewer bytes than the write to null.

It's not clear looking at the assembly why there would be any measurable performance difference either way.

Brett

[Nico Weber]

Does it differ further down? The compiler can probably figure out that IMMEDIATE_CRASH() won't return, but it can't figure this out for BreakDebugger (which can return, if you run chrome under a debugger on Windows). Maybe the knowledge that IMMEDIATE_CRASH() doesn't return helps make optimization decisions. If so, we could change CHECK to not call BreakDebugger().

[Brett Wilson]

On Tue, May 10, 2016 at 2:19 PM, Nico Weber <tha...@chromium.org> wrote:

Does it differ further down? The compiler can probably figure out that IMMEDIATE_CRASH() won't return, but it can't figure this out for BreakDebugger (which can return, if you run chrome under a debugger on Windows). Maybe the knowledge that IMMEDIATE_CRASH() doesn't return helps make optimization decisions. If so, we could change CHECK to not call BreakDebugger().

You made me realize something else: The compiler should be able to assume all registers are unchanged across the write to 0, but (almost) everything could have been invalidated by the function call, so the CHECK will act like a big barrier it can't optimize registers across.. Seems like this may be good to change.

Brett

[Antoine Labour]

Another thing: RELEASE_ASSERT hints the compiler that the condition is likely not true (the UNLIKELY()) and that's probably what helps it move the unlikely branch out of the way, and be more coherent for i-cache and branch prediction for the common path. That seems like a no-brainer too for CHECK/DCHECK.

Antoine

[Brett Wilson]

I thought that too (I'm also happy annotating the condition), but I concluded it probably won't matter. According to:

https://software.intel.com/en-us/articles/branch-and-loop-reorganization-to-prevent-mispredicts

a forward branch is predicted as not taken. But both RELEASE_ASSERT and CHECK generated normally-taken forward branches in my examples, which is backwards.

I think this is almost unavoidable. The code generates a condition and a one-instruction thing to do inside it. Except maybe in a few cases like end of a function where it can invert the condition and return early, this will always generate a conditional jump over the "assertion failed" case.

Brett

[Scott Graham]

Don't think it matters on Windows: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/wtf/Compiler.h&l=89

[Dale Curtis]

FWIW, the progenitor of Chrome's base/logging.h in the internal Google code base has some updates that Chrome is missing. Mostly the UNLIKELY() clause, plus a different ordering. Probably some of those might be worth bringing over.

- dale

[Kentaro Hara]

What's the current status of the RELEASE_ASSERT => CHECK replacement?

I think that the latest status is that we again had to revert the change due to the performance regression. Is it because the replacement still has some real regression on official builds? Or is it because the perf bots are not running official builds?

[Brett Wilson]

The performance issue should have been fixed in https://codereview.chromium.org/1937613002

but that was reverted a bit after in https://codereview.chromium.org/2046593002

because it broke arm64 breakpad crash detection.

It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it. From the revert commit message it doesn't seem very difficult to fix breakpad (just need to handle another signal). Conceivably we could condition the behavior on CPU architecture but that seems like an unfortunate variance best avoided if possible.

Brett

[Nico Weber]

On Tue, Jun 14, 2016 at 6:45 AM, Brett Wilson <bre...@chromium.org> wrote:

The performance issue should have been fixed in https://codereview.chromium.org/1937613002

(and https://codereview.chromium.org/1982123002/, which is the reverted bit)

 

but that was reverted a bit after in https://codereview.chromium.org/2046593002

because it broke arm64 breakpad crash detection.

It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it. From the revert commit message it doesn't seem very difficult to fix breakpad (just need to handle another signal). Conceivably we could condition the behavior on CPU architecture but that seems like an unfortunate variance best avoided if possible.

(Note that the revert reason means that we're currently not seeing all the crashes in blink from IMMEDIATE_CRASH, since as we now know, breakpad doesn't catch this signal. So once we fix breakpad, we'll likely see a whole bunch of crashes that we're currently missing – at least on platforms that are still on breakpad (everything except Mac and Windows I think). Crashpad gets this right I think.)

[Nico Weber]

On Tue, Jun 14, 2016 at 6:45 AM, Brett Wilson <bre...@chromium.org> wrote:

The performance issue should have been fixed in https://codereview.chromium.org/1937613002

but that was reverted a bit after in https://codereview.chromium.org/2046593002

because it broke arm64 breakpad crash detection.

It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it.

Looks like primiano is on top of this (https://codereview.chromium.org/2068673002/) :-)

[Primiano Tucci]

Yup my plan is to re-land Nico's https://codereview.chromium.org/1982123002/  once the breakpad change sticks and hits dev channel.

>It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it. From the revert commit message it doesn't seem very difficult to fix breakpad (just need to handle another signal).

Yeah the breakpad change itself it's a one line. It's just that every time we changed a small bit in the signal handling logic we unveiled crazy things happening in the wild *

So it's just a matter to figure out if somebody out there is doing something unexpected with SIGTRAP.

How urgent is this? My point of waiting for dev channel is to test the two things separately, so if we unexpectedly start losing crash reports on some platforms we at least know that's just due to something else interfering with SIGTRAP and not because of SIGTRAP + us relying on trap for CHECKs. 

The other option is to do both (breakpad change and the unrevert) now. But if something goes wrong we'll have to revert both and restart from scratch and won't know what caused what.

 * The last thing that comes on top of my head is a binary translator leveraging userspace handling of SEGV to switch between arm and x86 instruction sets. See crbug.com/477444

[Nico Weber]

On Tue, Jun 14, 2016 at 4:24 PM, Primiano Tucci <prim...@chromium.org> wrote:

Yup my plan is to re-land Nico's https://codereview.chromium.org/1982123002/  once the breakpad change sticks and hits dev channel.

>It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it. From the revert commit message it doesn't seem very difficult to fix breakpad (just need to handle another signal).

Yeah the breakpad change itself it's a one line. It's just that every time we changed a small bit in the signal handling logic we unveiled crazy things happening in the wild *

So it's just a matter to figure out if somebody out there is doing something unexpected with SIGTRAP.

How urgent is this? My point of waiting for dev channel is to test the two things separately, so if we unexpectedly start losing crash reports on some platforms we at least know that's just due to something else interfering with SIGTRAP and not because of SIGTRAP + us relying on trap for CHECKs. 

The other option is to do both (breakpad change and the unrevert) now. But if something goes wrong we'll have to revert both and restart from scratch and won't know what caused what.

I think it's fine to not rush this. Things are moving forward, which I think is probably all haraken wanted to know.

[Kentaro Hara]

On Tue, Jun 14, 2016 at 11:30 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Jun 14, 2016 at 4:24 PM, Primiano Tucci <prim...@chromium.org> wrote:

Yup my plan is to re-land Nico's https://codereview.chromium.org/1982123002/  once the breakpad change sticks and hits dev channel.

>It seems like breakpad needs an update. I'm not sure on what timescale that might happen or even if anybody is looking at it. From the revert commit message it doesn't seem very difficult to fix breakpad (just need to handle another signal).

Yeah the breakpad change itself it's a one line. It's just that every time we changed a small bit in the signal handling logic we unveiled crazy things happening in the wild *

So it's just a matter to figure out if somebody out there is doing something unexpected with SIGTRAP.

How urgent is this? My point of waiting for dev channel is to test the two things separately, so if we unexpectedly start losing crash reports on some platforms we at least know that's just due to something else interfering with SIGTRAP and not because of SIGTRAP + us relying on trap for CHECKs. 

The other option is to do both (breakpad change and the unrevert) now. But if something goes wrong we'll have to revert both and restart from scratch and won't know what caused what.

I think it's fine to not rush this. Things are moving forward, which I think is probably all haraken wanted to know.

Yes, I'm happy that you're moving the ball forward!

[Dana Jansens]

On Mon, Jun 13, 2016 at 9:45 PM, Brett Wilson <bre...@chromium.org> wrote:

The performance issue should have been fixed in https://codereview.chromium.org/1937613002

but that was reverted a bit after in https://codereview.chromium.org/2046593002

because it broke arm64 breakpad crash detection.

kotenkov ran some more perf tests with CHECK on June 1, which was after the above fix, and before the revert. They observed a regression/performance change with CHECK still: http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06-01_15-50-44.

(From https://codereview.chromium.org/1992873004/)

[Brett Wilson]

Just to be sure, these testes are running in official builds, right? Non-official builds will still have the backtrace/log message printing added which will prevent register optimization across the assertion (in addition to making the code slightly larger which is probably a lesser effect).

Somebody will have to compare the assembly.

Brett

[Dana Jansens]

On Thu, Jun 16, 2016 at 1:31 PM, Brett Wilson <bre...@chromium.org> wrote:

Just to be sure, these testes are running in official builds, right? Non-official builds will still have the backtrace/log message printing added which will prevent register optimization across the assertion (in addition to making the code slightly larger which is probably a lesser effect).

They're perf bots, so they certainly should be. Here's another example perf tryjob on windows (not this exact run):

https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1654

They appear to be using GYP still. The gclient runhooks step says:

GYP_DEFINES: branding=Chrome buildtype=Official chromium_win_pch=0 component=static_library fastbuild=1 gomadir='C:\b\build\slave\cache\cipd\goma' target_arch=x64 use_goma=1

https://build.chromium.org/p/tryserver.chromium.perf/builders/winx64nvidia_perf_bisect/builds/1654/steps/gclient%20runhooks%20%28with%20patch%29/logs/stdio

[Brett Wilson]

With Nico's patch for logging https://codereview.chromium.org/1982123002

I added a check to the top of base/run_all_unittests.exe and did an official build. I get:

  CHECK(argc >= 0) << "foo";

00E219F6 85 F6                test        esi,esi  

00E219F8 79 07                jns         main+27h (0E21A01h)  

00E219FA C6 05 00 00 00 00 00 mov         byte ptr ds:[0],0  

In RunAllTests.cpp for webkit_unit_tests I added a similar line:

    RELEASE_ASSERT(testSuite);

0232239C 8B 4D 08             mov         ecx,dword ptr [testSuite]  

0232239F 85 C9                test        ecx,ecx  

023223A1 75 06                jne         `anonymous namespace'::runHelper+1Dh (023223A9h)  

023223A3 88 0D 00 00 00 00    mov         byte ptr ds:[0],cl  

In this case the parameter isn't already in a register which accounts for the extra first instruction. In this case the compiler can cleverly deduce that when the condition fails, cl will be 0 and it can save one byte in the instruction coding by moving cl to 0 rather than a 32-bit constant.

In 64-bit Nico's base version looks the same:

  CHECK(argc >= 0) << "foo";

000000013F9C0C8D 85 C9                test        ecx,ecx  

000000013F9C0C8F 79 08                jns         main+35h (013F9C0C99h)  

000000013F9C0C91 C6 04 25 00 00 00 00 00 mov         byte ptr [0],0  

    RELEASE_ASSERT(testSuite);

000000013FD7B864 48 85 FF             test        rdi,rdi  

000000013FD7B867 75 08                jne         `anonymous namespace'::runHelper+31h (013FD7B871h)  

000000013FD7B869 40 88 3C 25 00 00 00 00 mov         byte ptr [0],dil  

Unless there is something weird going on with the optimizer when used in different contexts, I don't see any difference between these implementations.

Brett

[Primiano Tucci]

Closing the loop on this. The breakpad support for SIGTRAP landed and seems to have stuck.

I'm relanding Nico's logging patch right now in https://codereview.chromium.org/2125923002/

[TAMURA, Kent]

I had some investigation for Windows-only regression, crbug.com/614852, even with Nico's change.

In Windows official x86 VC++ build, RELEASE_ASSERT(cond) and CHECK(cond) produced different instructions in function prologue and epilogue.

RELEASE_ASSERT:

  00000000: 55                 push        ebp

  00000001: 8B EC              mov         ebp,esp

  00000003: 53                 push        ebx

  00000004: 8B 5D 08           mov         ebx,dword ptr [ebp+8]

  00000007: 56                 push        esi

  00000008: 8B F1              mov         esi,ecx

  0000000A: 57                 push        edi

  0000000B: 3B 5E 08           cmp         ebx,dword ptr [esi+8]

  0000000E: 76 07              jbe         00000017

  00000010: C6 05 00 00 00 00  mov         byte ptr ds:[0],0

            00

  ...

  00000053: 5F                 pop         edi

  00000054: 5E                 pop         esi

  00000055: 5B                 pop         ebx

  00000056: 5D                 pop         ebp

  00000057: C2 08 00           ret         8

CHECK:

  00000000: 55                 push        ebp

  00000001: 8B EC              mov         ebp,esp

  00000003: 51                 push        ecx

  00000004: 83 65 FC 00        and         dword ptr [ebp-4],0

  00000008: 53                 push        ebx

  00000009: 8B 5D 08           mov         ebx,dword ptr [ebp+8]

  0000000C: 56                 push        esi

  0000000D: 8B F1              mov         esi,ecx

  0000000F: 57                 push        edi

  00000010: 3B 5E 08           cmp         ebx,dword ptr [esi+8]

  00000013: 76 07              jbe         0000001C

  00000015: C6 05 00 00 00 00  mov         byte ptr ds:[0],0

            00

  ...

  00000058: 5F                 pop         edi

  00000059: 5E                 pop         esi

  0000005A: 5B                 pop         ebx

  0000005B: 8B E5              mov         esp,ebp

  0000005D: 5D                 pop         ebp

  0000005E: C2 08 00           ret         8

This [ebp-4] is not referred in this function at all.  If CHECK is used in functions which are called very frequently, the cost of this push/and/mov affects production performance.

I modified CHECK implementation as the following locally, and confirmed the modified implementation didn't produce the unnecessary instructions.

#define CHECK(condition)                                                \

  if (!(condition)) LOGGING_CRASH();

I wonder if we can eliminate the unnecessary instructions by changing compiler flags.

[TAMURA, Kent]

More investigation.

The following |func1()| produces unnecessary instructions.

class LM {

 public:

  LM();

  ~LM();

};

int func1() {

    true ? (void) 0 : LM();

    return 15;

}

  00000000: 55                 push        ebp

  00000001: 8B EC              mov         ebp,esp

  00000003: 51                 push        ecx

  00000004: 83 65 FC 00        and         dword ptr [ebp-4],0

  00000008: 6A 0F              push        0Fh

  0000000A: 58                 pop         eax

  0000000B: 8B E5              mov         esp,ebp

  0000000D: 5D                 pop         ebp

  0000000E: C3                 ret

Removing ~LM() or making it |~LM() = default| resolves the issue.

[Brett Wilson]

My guess is that this has to do with us using "/Oy-" (in build/config/compiler/BUILD.gn) which disables omitting frame pointers. I think we do this to make the crash dumps usable. It might be interesting to see if the same thing happens with this flag commented-out.

I suspect we can not remove the /Oy-. This type of thing may also affect how we see assertion failures in crash dumps. I would want them to be clearly identifiable and separable from random null dereferences. I wonder if the instructions affect this? 

Does somebody have an example of what a Blink assertion failure looks like on the crash serbver?

Brett

[TAMURA, Kent]

Unfortunately, /Oy- didn't help.  I tried several optimization flags, and I couldn't find a way to remove the unnecessary instructions.

We should try Visual Studio 2017 RC, and should report to Microsoft if it doesn't fix the issue.

[TAMURA, Kent]

I confirmed VS2017RC still have the issue, and reported to Microsoft.

[prim...@google.com]

Is there a clear repro which puts me in a place where CHECK produces different assembly than RELEASE_ASSERT ?

I can't repro what is claimed above in this thread:

class LM {
 public:
  LM();
  ~LM();
};
int func1() {
    true ? (void) 0 : LM();
    return 15;
}
  00000000: 55                 push        ebp
  00000001: 8B EC              mov         ebp,esp
  00000003: 51                 push        ecx
  00000004: 83 65 FC 00        and         dword ptr [ebp-4],0
  00000008: 6A 0F              push        0Fh
  0000000A: 58                 pop         eax
  0000000B: 8B E5              mov         esp,ebp
  0000000D: 5D                 pop         ebp
  0000000E: C3                 ret

I am trying https://codereview.chromium.org/2532513002/ on win (ToT, so using whatever version of MSVC depot_tools uses today).

These gn args:

is_component_build = false is_debug = false is_official_build = true

int func1() { true ? (void) 0 : LM(); return 15; }

I get:

0:000> uf check_example!func1

check_example!func1 [d:\src\chrome\src\base\check_example.cc @ 33]:

   33 00007ff6`30cf5794 8364240800      and     dword ptr [rsp+8],0

   35 00007ff6`30cf5799 b80f000000      mov     eax,0Fh

   36 00007ff6`30cf579e c3              ret

More in general I can't get to a state where the two generate different asm

Still from  https://codereview.chromium.org/2532513002/ 

0:000> uf check_example!DoCheck

   17 00007ff6`30cf5780 8364240800      and     dword ptr [rsp+8],0

   18 00007ff6`30cf5785 85c9            test    ecx,ecx

   18 00007ff6`30cf5787 7507            jne     check_example!DoCheck+0x10 (00007ff6`30cf5790) 

   18 00007ff6`30cf5789 880c2500000000  mov     byte ptr [0],cl

   19 00007ff6`30cf5790 c3              ret  

0:000> uf check_example!DoBlinkRelAssert

   22 00007ff6`30cf5774 85c9            test    ecx,ecx

   23 00007ff6`30cf5776 7507            jne     check_example!DoBlinkRelAssert+0xb (00007ff6`30cf577f) 

   23 00007ff6`30cf5778 880c2500000000  mov     byte ptr [0],cl

   24 00007ff6`30cf577f c3              ret  

On Tuesday, November 22, 2016 at 9:25:19 AM UTC, Kent Tamura wrote:

This one: https://connect.microsoft.com/VisualStudio/Feedback/Details/3112569

I don't seem able to open that. I guess it takes an MSDN subscription. Is there a summary available somewhere else?

I am wondering whether that push ecx that you see is an effect of the ctor/dtor implicitly using a thiscall convention and MSVC emitting the push ecx (+cleanup) before realizing that there is no need to call the ctor/dtor.

In that case I wanted to try whether just changing the calling convention of LM's ctor/dtor (and in the real case LogMessageVoidify's ctor/dtor) could fix it.

However I'm not able to get into that state (i.e. seeing the compiler emitting the extra push ecx instructions) in the first place.

[Will Harris]

Hello,

While debugging the root cause of a crash signature, I found this thread linked from https://codereview.chromium.org/1982123002/ which is the CL that seems to have changed the behavior of CHECK from a __debugbreak() to a crash on NULL ptr write.

Now, the code seems it no longer does an INT 3 and instead does a MOV byte ptr [(00000000`00000000)],0

INT3 is one instruction (CC) but this code is eight (C6 04 25 00 00 00 00 00) - so I wonder why this was changed?

In particular, there is now seemingly no longer any way to distinguish in histograms such as CrashExitCodes, and in Crash/ frontend, the difference between CHECK and a ACCESS_VIOLATION crash. This is an important loss of data since CrashExitCode is one of the primary mechanism by which we can subdivide child process crashes from UMA and pair them up with crash dumps e.g. to measure the % of crashes due to a CHECK rather than other types.

Will

[Primiano Tucci]

Nico would remember more, but I think the thing that drove his CL was the fact that BreakDebugger() is *not* a noreturn function and was causing poor codegen in some cases.

Having said that, we had some somewhat related discussion on this CL of mine (note: is *not* landed yet). That CL addresses a completely different problem, and I was not planning to touch windows there. But since you found this issue (that caused some similar issues on arm64 as you can see form the revert) I wonder if we could do something similar.

(I don't know concretely if MSVC supports statement expressions, which are supposed to be a GCC extension I used in that CL).

[Will Harris]

Hmm okay - I'll have a look through the CL and see if I can find more info on the decision. FWIW it seems MSVC supports noreturn attribute e.g.

https://msdn.microsoft.com/en-us/library/k6ktzx3s.aspx

I'm sympathetic to the size/codegen issues and I'm happy with any solution... but not having CHECK() return a EXCEPTION_BREAKPOINT return code from a child process is a regression that makes analysis harder, so I've raised a bug - https://crbug.com/687326 - to track this. When it's fixed we should add death tests to make sure this doesn't regress again...

Thanks,

Will

[Torne (Richard Coles)]

It's not that we can't mark it noreturn because the toolchain doesn't support it, it's that BreakDebugger actually *can* return (when you have a debugger attached), and so marking it noreturn would break things.

[Scott Hess]

The same is true of assigning to a NULL pointer, though, so that
doesn't seem like a strong argument.

-scott

[Primiano Tucci]

On Wed, Feb 1, 2017 at 7:53 AM Scott Hess <sh...@chromium.org> wrote:

The same is true of assigning to a NULL pointer, though, so that
doesn't seem like a strong argument.

But assigning a null pointer doesn't involve setting up a call frame. IIRC the problem there was coming from setting up a full call frame  to call BreakDebugger, because is not no-return (otherwise a jmp would have been roughly as expensive as dereferencing a ptr). 

Anyways, since this problem came up, I will give it a go to see if the strategy of https://codereview.chromium.org/2502953003/ can be used also with MSVC. (TL;DR emitting the asm instructions that cause a trap without calling a function).

It really boils down to:

- does MSVC support statement expressions (or something similar)

- will that cause any side-effect (in terms of binary / perf) I am not thinking about? 

[Scott Graham]

On Wed, Feb 1, 2017 at 10:00 AM, Primiano Tucci <prim...@google.com> wrote:

On Wed, Feb 1, 2017 at 7:53 AM Scott Hess <sh...@chromium.org> wrote:

The same is true of assigning to a NULL pointer, though, so that
doesn't seem like a strong argument.

But assigning a null pointer doesn't involve setting up a call frame. IIRC the problem there was coming from setting up a full call frame  to call BreakDebugger, because is not no-return (otherwise a jmp would have been roughly as expensive as dereferencing a ptr). 

Anyways, since this problem came up, I will give it a go to see if the strategy of https://codereview.chromium.org/2502953003/ can be used also with MSVC. (TL;DR emitting the asm instructions that cause a trap without calling a function).

It really boils down to:

- does MSVC support statement expressions (or something similar)

No, has to be do {} while(0) or similar.

 

- will that cause any side-effect (in terms of binary / perf) I am not thinking about? 

Is it BreakDebugger() that's the problem, or __debugbreak()? I would try the latter first, as it should be "pretty equivalent" to __asm { int 3 }.

[Primiano Tucci]

On Wed, Feb 1, 2017 at 10:11 AM Scott Graham <sco...@chromium.org> wrote:

On Wed, Feb 1, 2017 at 10:00 AM, Primiano Tucci <prim...@google.com> wrote:

On Wed, Feb 1, 2017 at 7:53 AM Scott Hess <sh...@chromium.org> wrote:

The same is true of assigning to a NULL pointer, though, so that
doesn't seem like a strong argument.

But assigning a null pointer doesn't involve setting up a call frame. IIRC the problem there was coming from setting up a full call frame  to call BreakDebugger, because is not no-return (otherwise a jmp would have been roughly as expensive as dereferencing a ptr). 

Anyways, since this problem came up, I will give it a go to see if the strategy of https://codereview.chromium.org/2502953003/ can be used also with MSVC. (TL;DR emitting the asm instructions that cause a trap without calling a function).

It really boils down to:

- does MSVC support statement expressions (or something similar)

No, has to be do {} while(0) or similar.

 

- will that cause any side-effect (in terms of binary / perf) I am not thinking about? 

Is it BreakDebugger() that's the problem, or __debugbreak()?

Definitely BreakDebugger(). Do you know if __debugbreak is codegen-ed as a function call os is the equivalent of __builtin_trap?

 

I would try the latter first, as it should be "pretty equivalent" to __asm { int 3 }.

If __debugbreak() is equivalent to a trap and there are no statements expression support it might end up being the only option to fix this on windows.

For context, the reason why on Linux/Android I'm manually asm insteaf of __builtin_trap (which sounds like the gcc equivalent of __debugbreak) is because of crbug.com/664209 (TL;DR __builtin_trap are too aggressively optimized making it impossible to distinguish different CHECKs within the same function).

Need to check what would happens in the same scenario (See example on crbug.com/664209#c0) on windows. 

[Scott Graham]

I commented on crbug.com/664209. Maybe the easiest thing to do is ping me when you're happy with non-Windows, and I will take a look having Windows behave the same way (crbug.com/687326 I guess).