Implement Dump-on-DCHECK (via a new LogSeverity).

Implement Dump-on-DCHECK (via a new LogSeverity).

This patch adds a new LogSeverity value, LOG_DUMP, available in all builds. LOG_DUMP has similar semantics to LOG_FATAL, except that it uploads a dump without actually crashing, and only does so on the first DUMP-severity failure.

A new DCHECK_IS_DUMP_WITHOUT_CRASHING macro is introduced, which modifies the behaviour of the LOG_DFATAL/LOG_DCHECK severity in DCHECK_IS_ON()[1] builds, such that these severities now map to:

  !DCHECK_IS_ON() (e.g. Release)                   -> LOG_ERROR
  DCHECK_IS_ON()                                   -> LOG_FATAL
  DCHECK_IS_ON() + DCHECK_IS_DUMP_WITHOUT_CRASHING -> LOG_DUMP

A new build argument, dump_on_first_dcheck, is set in the GN configuration to enable the DCHECK_IS_DUMP_WITHOUT_CRASHING macro.

All non-DCHECK_IS_ON() behaviours, e.g. CHECK, PCHECK, SECURITY_CHECK, are unchanged and continue to be mapped to the LOG_FATAL severity.

Dump-on-DCHECK will be enabled in on-off Canary builds by temporarily landing patches to set dump_on_first_dcheck in specific configurations.  This underlying LOG_DUMP implementation is intended to remain checked-in indefinitely.

BUG=596231
[1] DCHECK_IS_ON() is set in Debug builds, and Release builds with DCHECK_ALWAYS_ON.

[Wez, 2016-08-27 08:39:39]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 08:40:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 09:28:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-27 09:28:11]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Wez, 2016-08-27 19:42:10]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-27 19:42:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 20:34:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-27 20:34:17]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Wez, 2016-08-28 04:31:31]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 04:31:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-28 04:37:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-28 04:37:32]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Wez, 2016-08-29 17:40:10]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-29 17:52:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-08-29 23:07:02]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 00:01:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:41:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 03:41:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wez, 2016-11-22 23:01:08]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:01:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 23:06:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:06:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Wez, 2016-11-22 23:22:48]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:23:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 00:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 00:31:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wez, 2016-12-17 06:15:19]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-17 06:15:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 06:17:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-17 06:17:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Wez, 2016-12-17 07:36:40]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-17 07:36:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 07:45:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-17 07:45:17]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Wez, 2016-12-19 23:36:23]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 23:36:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-12-19 23:57:45]

wez@chromium.org changed reviewers:
+ danakj@chromium.org, esprehn@chromium.org, groby@chromium.org,
kmarshall@chromium.org, wfh@google.com

[Wez, 2016-12-19 23:57:46]

PTAL: Comments on the latest patch set include questions which it'd be good to
get some input on.

One of the major questions is whether we should be more consistent in how we
gloss-over death tests and DCHECK death tests on platforms & configurations
which can't support them; we are presently not very consistent across the
codebase and I think it'd be helpful to provide a more consistent mechanism for
skipping these tests where they don't make sense.

There is a separate CL which force-enables the flags, at crrev.com/2591583002,
though it fails try-bots right now because of the DCHECK death-test issues.

https://codereview.chromium.org/2288473002/diff/160001/base/debug/dump_withou...
File base/debug/dump_without_crashing.h (right):

https://codereview.chromium.org/2288473002/diff/160001/base/debug/dump_withou...
base/debug/dump_without_crashing.h:21: BASE_EXPORT bool DumpWithoutCrashing();
Some new call-sites have been introduced lately which Bind() this, assuming it
returns void.

Q: Would it be cleaner to add an explicit Dump1stTimeWithoutCrashing()?

Q: To ensure test reliability, should we also have ResetDump1stTimeForTest()?

https://codereview.chromium.org/2288473002/diff/160001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2288473002/diff/160001/base/logging.cc#newcod...
base/logging.cc:744: if (severity_ == LOG_DUMP) {
Q: Is it more or less readable to have LOG_DUMP implemented in the middle of the
LOG_FATAL code like this?

https://codereview.chromium.org/2288473002/diff/160001/base/logging.cc#newcod...
base/logging.cc:750: // dump per-racing-thread, rather than our aim of one
per-process.
Note to self: Fix comment line-wrap.

https://codereview.chromium.org/2288473002/diff/160001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2288473002/diff/160001/base/logging.h#newcode300
base/logging.h:300: const LogSeverity LOG_NUM_SEVERITIES = 5;
Q: Is changing the number of severities acceptable?

Q: Does it make sense for DUMP to have high "severity" than FATAL?

Q: Should DUMP be DUMP1ST, since it only dumps on the first time it is
triggered?

https://codereview.chromium.org/2288473002/diff/160001/base/metrics/sample_ve...
File base/metrics/sample_vector_unittest.cc (right):

https://codereview.chromium.org/2288473002/diff/160001/base/metrics/sample_ve...
base/metrics/sample_vector_unittest.cc:137:
EXPECT_DCHECK_DEATH(samples.Accumulate(INT_MAX, 100));
Q: Having DCHECK death-tests run but trivially pass doesn't seem helpful.  Can
we instead update the test infrastructure to be aware of death-tests and
DCHECK-tests - e.g:

DEATH_FooTest -> Run only if death tests are supported.
DCHECK_DEATH_FooTest -> Run only if DCHECK causes death on failure.

We could alternatively implement that via macros:

#define DCHECK_DEATH_NAME(test) ...

but that does make test names annoyingly verbose.

https://codereview.chromium.org/2288473002/diff/160001/base/test/gtest_util.h
File base/test/gtest_util.h (right):

https://codereview.chromium.org/2288473002/diff/160001/base/test/gtest_util.h...
base/test/gtest_util.h:24: #if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST)
&& !defined(OS_ANDROID) && !defined(DCHECK_IS_DUMP_WITHOUT_CRASH)
This causes EXPECT_DCHECK_DEATH to be a no-op if DCHECK is defined to
dump-on-DCHECK. For some tests we could still have them work, and just verify
that dump-without-crashing gets called.  For others the DCHECKs actually protect
against e.g. C++ data-structure corruption, so we'll crash anyway if we don't
have DCHECK crash.

Q: Is there any down-side to losing this test coverage in dump-on-DCHECK builds?

https://codereview.chromium.org/2288473002/diff/160001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/2288473002/diff/160001/build/config/BUILD.gn#...
build/config/BUILD.gn:70: defines += [ "DCHECK_IS_DUMP_WITHOUT_CRASH=1" ]
Q: Suggestions on better naming welcome!

https://codereview.chromium.org/2288473002/diff/160001/build/config/dcheck_al...
File build/config/dcheck_always_on.gni (right):

https://codereview.chromium.org/2288473002/diff/160001/build/config/dcheck_al...
build/config/dcheck_always_on.gni:10: # uploaded, without crashing. Implies
dcheck_always_on, but does not
Note to self: Correct implies->requires

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Assertions.h:160: #define ASSERT_NOT_REACHED()
NOTREACHED()
Note that this has significant performance implications. Yay.

[commit-bot: I haz the power, 2016-12-20 00:40:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:40:17]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[esprehn, 2016-12-20 03:16:29]

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Assertions.h:160: #define ASSERT_NOT_REACHED()
NOTREACHED()
On 2016/12/19 at 23:57:46, Wez wrote:
> Note that this has significant performance implications. Yay.

What was the perf change? Not sure I understand how switching ASSERT_NOT_REACHED
changed the perf? Note that blink will probably switch all of these to
NOTREACHED() someday so it'd be good to understand what changed?

[danakj, 2016-12-20 15:09:36]

On Mon, Dec 19, 2016 at 6:57 PM, <wez@chromium.org> wrote:

> Reviewers: esprehn, groby, wfh5, danakj_OOO_and_slow, Kevin M
> CL: https://codereview.chromium.org/2288473002/
>
> Message:
> PTAL: Comments on the latest patch set include questions which it'd be
> good to
> get some input on.
>
> One of the major questions is whether we should be more consistent in how
> we
> gloss-over death tests and DCHECK death tests on platforms & configurations
> which can't support them; we are presently not very consistent across the
> codebase and I think it'd be helpful to provide a more consistent
> mechanism for
> skipping these tests where they don't make sense.
>

We have one now, but it's not widely used outside of base yet.
https://cs.chromium.org/chromium/src/base/test/gtest_util.h?rcl=0&l=17

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-12-20 15:09:36]

On Mon, Dec 19, 2016 at 6:57 PM, <wez@chromium.org> wrote:

> Reviewers: esprehn, groby, wfh5, danakj_OOO_and_slow, Kevin M
> CL: https://codereview.chromium.org/2288473002/
>
> Message:
> PTAL: Comments on the latest patch set include questions which it'd be
> good to
> get some input on.
>
> One of the major questions is whether we should be more consistent in how
> we
> gloss-over death tests and DCHECK death tests on platforms & configurations
> which can't support them; we are presently not very consistent across the
> codebase and I think it'd be helpful to provide a more consistent
> mechanism for
> skipping these tests where they don't make sense.
>

We have one now, but it's not widely used outside of base yet.
https://cs.chromium.org/chromium/src/base/test/gtest_util.h?rcl=0&l=17

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2016-12-20 18:29:36]

Yes, I'm familiar with EXPECT_DCHECK_DEATH (the latest patch-set updates a
couple of tests to use it, in fact).  The problem with it is that it
band-aids death-tests that simply shouldn't run at all, to (1) pass even
though they tested nothing meaningful and (2) spam the log with a message
indicating that fact.

The unit-test framework already has support for tests being skipped if they
start with DISABLED_m, so rather than have no-op(ish) tests, why not have a
DEATH_TEST(foo), MAYBE_DEATH(foo) or whatever macro, which expands to
DISABLED_foo in configurations that don't support death-tests?

On 20 December 2016 at 07:09, <danakj@chromium.org> wrote:

> On Mon, Dec 19, 2016 at 6:57 PM, <wez@chromium.org> wrote:
>
>> Reviewers: esprehn, groby, wfh5, danakj_OOO_and_slow, Kevin M
>> CL: https://codereview.chromium.org/2288473002/
>>
>> Message:
>> PTAL: Comments on the latest patch set include questions which it'd be
>> good to
>> get some input on.
>>
>> One of the major questions is whether we should be more consistent in how
>> we
>> gloss-over death tests and DCHECK death tests on platforms &
>> configurations
>> which can't support them; we are presently not very consistent across the
>> codebase and I think it'd be helpful to provide a more consistent
>> mechanism for
>> skipping these tests where they don't make sense.
>>
>
> We have one now, but it's not widely used outside of base yet.
> https://cs.chromium.org/chromium/src/base/test/gtest_util.h?rcl=0&l=17
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2016-12-20 18:29:38]

Yes, I'm familiar with EXPECT_DCHECK_DEATH (the latest patch-set updates a
couple of tests to use it, in fact).  The problem with it is that it
band-aids death-tests that simply shouldn't run at all, to (1) pass even
though they tested nothing meaningful and (2) spam the log with a message
indicating that fact.

The unit-test framework already has support for tests being skipped if they
start with DISABLED_m, so rather than have no-op(ish) tests, why not have a
DEATH_TEST(foo), MAYBE_DEATH(foo) or whatever macro, which expands to
DISABLED_foo in configurations that don't support death-tests?

On 20 December 2016 at 07:09, <danakj@chromium.org> wrote:

> On Mon, Dec 19, 2016 at 6:57 PM, <wez@chromium.org> wrote:
>
>> Reviewers: esprehn, groby, wfh5, danakj_OOO_and_slow, Kevin M
>> CL: https://codereview.chromium.org/2288473002/
>>
>> Message:
>> PTAL: Comments on the latest patch set include questions which it'd be
>> good to
>> get some input on.
>>
>> One of the major questions is whether we should be more consistent in how
>> we
>> gloss-over death tests and DCHECK death tests on platforms &
>> configurations
>> which can't support them; we are presently not very consistent across the
>> codebase and I think it'd be helpful to provide a more consistent
>> mechanism for
>> skipping these tests where they don't make sense.
>>
>
> We have one now, but it's not widely used outside of base yet.
> https://cs.chromium.org/chromium/src/base/test/gtest_util.h?rcl=0&l=17
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Wez, 2016-12-22 01:32:23]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 01:32:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2016-12-22 01:33:12]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 01:33:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 02:46:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 02:46:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[danakj, 2016-12-22 19:05:58]

Description was changed from

==========
Simplify dump-on-DCheck

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process. The implementation follows the approach taken for CHECK & CHECK_OP
in official-branded builds, and strips out any log messages associated with the
check.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

to

==========
Simplify dump-on-DCheck

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process. The implementation follows the approach taken for CHECK & CHECK_OP
in official-branded builds, and strips out any log messages associated with the
check.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

[danakj, 2016-12-22 19:05:58]

danakj@chromium.org changed reviewers:
+ gab@chromium.org

[danakj, 2016-12-22 19:09:35]

On 2016/12/20 18:29:38, Wez wrote:
> Yes, I'm familiar with EXPECT_DCHECK_DEATH (the latest patch-set updates a
> couple of tests to use it, in fact).  The problem with it is that it
> band-aids death-tests that simply shouldn't run at all, to (1) pass even
> though they tested nothing meaningful and (2) spam the log with a message
> indicating that fact.
> 
> The unit-test framework already has support for tests being skipped if they
> start with DISABLED_m, so rather than have no-op(ish) tests, why not have a
> DEATH_TEST(foo), MAYBE_DEATH(foo) or whatever macro, which expands to
> DISABLED_foo in configurations that don't support death-tests?

I see, that's a good point; tests may have other expectations in them than the
death check but hopefully that's all any given death test is really testing. The
logic for EXPECT_DCHECK_DEATH being enabled or not is all in the preprocessor so
I think that would work. However, what if GTEST_HAS_DEATH_TEST is not defined.
Will EXPECT_DEATH compile? The way it is done now the EXPECT_DEATH is not part
of the build when it's disabled, but I think what you're saying below it would,
and I'm not sure if that would work. It would look like this?

TEST(ThingerTest, DEATH_TEST(CrashAsExpected)) {
  ...
}

TEST_F(SubclassThingerTest, DEATH_TEST(CrashAsExpected)) {
  ...
}

+gab who wrote the EXPECT_DCHECK_DEATH macro.

> 
> On 20 December 2016 at 07:09, <mailto:danakj@chromium.org> wrote:
> 
> > On Mon, Dec 19, 2016 at 6:57 PM, <mailto:wez@chromium.org> wrote:
> >
> >> Reviewers: esprehn, groby, wfh5, danakj_OOO_and_slow, Kevin M
> >> CL: https://codereview.chromium.org/2288473002/
> >>
> >> Message:
> >> PTAL: Comments on the latest patch set include questions which it'd be
> >> good to
> >> get some input on.
> >>
> >> One of the major questions is whether we should be more consistent in how
> >> we
> >> gloss-over death tests and DCHECK death tests on platforms &
> >> configurations
> >> which can't support them; we are presently not very consistent across the
> >> codebase and I think it'd be helpful to provide a more consistent
> >> mechanism for
> >> skipping these tests where they don't make sense.
> >>
> >
> > We have one now, but it's not widely used outside of base yet.
> > https://cs.chromium.org/chromium/src/base/test/gtest_util.h?rcl=0&l=17
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[danakj, 2016-12-22 19:11:46]

Another possibility would be to make EXPECT_DCHECK_DEATH disabled when this CL
is active (windows official builds)?

[gab, 2016-12-22 20:07:07]

On 2016/12/22 19:11:46, danakj_OOO_and_slow wrote:
> Another possibility would be to make EXPECT_DCHECK_DEATH disabled when this CL
> is active (windows official builds)?

I'm fine with that.

Otherwise the idea was to still provide as much coverage as we could on
platforms that don't support death tests by running the rest of the code
anyways. It might not have much value in practice though given the main thing
under test isn't executed.
It'd at least make sense to only spew the warning about death tests not being
enabled once instead of for every test?

[Wez, 2017-01-07 00:18:47]

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/2288473002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Assertions.h:160: #define ASSERT_NOT_REACHED()
NOTREACHED()
On 2016/12/20 03:16:29, esprehn wrote:
> On 2016/12/19 at 23:57:46, Wez wrote:
> > Note that this has significant performance implications. Yay.
> 
> What was the perf change? Not sure I understand how switching
ASSERT_NOT_REACHED
> changed the perf? Note that blink will probably switch all of these to
> NOTREACHED() someday so it'd be good to understand what changed?

Earlier versions of the patch left Blink assertions disabled, to avoid the
related book-keeping overhead.

[Wez, 2017-01-07 00:20:37]

On 2016/12/22 20:07:07, gab wrote:
> On 2016/12/22 19:11:46, danakj_OOO_and_slow wrote:
> > Another possibility would be to make EXPECT_DCHECK_DEATH disabled when this
CL
> > is active (windows official builds)?
> 
> I'm fine with that.

That's actually what I've done in this patch (see the change in gtest_util.h).
:)

> Otherwise the idea was to still provide as much coverage as we could on
> platforms that don't support death tests by running the rest of the code
> anyways. It might not have much value in practice though given the main thing
> under test isn't executed.
> It'd at least make sense to only spew the warning about death tests not being
> enabled once instead of for every test?

[gab, 2017-01-09 18:12:20]

On 2017/01/07 00:20:37, Wez wrote:
> On 2016/12/22 20:07:07, gab wrote:
> > On 2016/12/22 19:11:46, danakj_OOO_and_slow wrote:
> > > Another possibility would be to make EXPECT_DCHECK_DEATH disabled when
this
> CL
> > > is active (windows official builds)?
> > 
> > I'm fine with that.
> 
> That's actually what I've done in this patch (see the change in gtest_util.h).
> :)

Thanks, tweak to gtest_util.h lgtm, let me know if I should look at anything
else.

> 
> > Otherwise the idea was to still provide as much coverage as we could on
> > platforms that don't support death tests by running the rest of the code
> > anyways. It might not have much value in practice though given the main
thing
> > under test isn't executed.
> > It'd at least make sense to only spew the warning about death tests not
being
> > enabled once instead of for every test?

[Wez, 2017-01-09 21:55:36]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 21:56:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 22:01:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 22:01:17]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Wez, 2017-01-09 22:29:53]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[Wez, 2017-01-09 22:30:20]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 22:30:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:18:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 23:18:22]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Wez, 2017-01-14 09:49:34]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 09:49:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-14 09:51:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 09:51:13]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Wez, 2017-01-15 01:01:34]

Description was changed from

==========
Simplify dump-on-DCheck

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process. The implementation follows the approach taken for CHECK & CHECK_OP
in official-branded builds, and strips out any log messages associated with the
check.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

to

==========
Implement Dump-on-DCHECK, based on dump_on_first_dcheck build argument.

This patch adds a new LogSeverity value, LOG_DUMP, available in all builds.
LOG_DUMP has similar semantics to LOG_FATAL, except that it uploads a dump
without actually crashing, and only does so on the first DUMP-severity failure.

A new DCHECK_IS_DUMP_WITHOUT_CRASHING macro is introduced, which modifies the
behaviour of the LOG_DFATAL/LOG_DCHECK severity in DCHECK_IS_ON()[1] builds,
such that these severities now map to:

  !DCHECK_IS_ON() (e.g. Release)                   -> LOG_ERROR
  DCHECK_IS_ON()                                   -> LOG_FATAL
  DCHECK_IS_ON() + DCHECK_IS_DUMP_WITHOUT_CRASHING -> LOG_DUMP

A new build argument, dump_on_first_dcheck, is set in the GN configuration to
enable the DCHECK_IS_DUMP_WITHOUT_CRASHING macro.

All non-DCHECK_IS_ON() behaviours, e.g. CHECK, PCHECK, SECURITY_CHECK, are
unchanged and continue to be mapped to the LOG_FATAL severity.

Dump-on-DCHECK will be enabled in on-off Canary builds by temporarily landing
patches to set dump_on_first_dcheck in specific configurations.  This underlying
LOG_DUMP implementation is intended to remain checked-in indefinitely.

BUG=596231

[1] DCHECK_IS_ON() is set in Debug builds, and Release builds with
DCHECK_ALWAYS_ON.
==========

[Wez, 2017-01-15 01:18:47]

Working implementation, with passing tests (see try-bot results in
crrev.com/2591583002). :)

https://codereview.chromium.org/2288473002/diff/320001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2288473002/diff/320001/base/logging.cc#newcod...
base/logging.cc:753: static bool dump_on_dcheck = true;
gab, danakj: Perhaps this should be a global so we can provide a
ResetDumpOnDCheckForTest(), so it's safe to run multiple-DUMP tests in a single
process; except right now we only have a single test. WDYT?

https://codereview.chromium.org/2288473002/diff/320001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2288473002/diff/320001/base/logging.h#newcode300
base/logging.h:300: const LogSeverity LOG_DUMP = 4;
gab, danakj: I'm concerned about adding this new LogSeverity, and expect that
there will be a load of sites that don't handle the new value (which should be
treated similarly to LOG_FATAL or LOG_ERROR in most cases).

How would you feel about moving the non-verbose log severities into an enum, so
that most call-sites can continue to use the integer form, but call-sites which
expect a non-verbose severity and switch based on it can be made checked more
easily?

https://codereview.chromium.org/2288473002/diff/320001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2288473002/diff/320001/base/logging_unittest....
base/logging_unittest.cc:225: #if !defined(DCHECK_IS_DUMP_WITHOUT_CRASH)
gab, danakj: I've completely disabled this test, since it relies on the
LogAssertHandler being called, which isn't the case with LOG(DUMP). Getting this
test to work with dump-on-DCHECK doesn't seem worth the additional
implementation complexity. WDYT?

https://codereview.chromium.org/2288473002/diff/320001/base/syslog_logging.cc
File base/syslog_logging.cc (right):

https://codereview.chromium.org/2288473002/diff/320001/base/syslog_logging.cc...
base/syslog_logging.cc:16: #undef LOG_WARNING
Yup, our LogSeverity name for warnings happens to clash with syslog.h-defined
macro. D'oh.

[Wez, 2017-01-15 01:20:06]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-15 01:20:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-01-15 01:29:14]

wez@chromium.org changed reviewers:
+ thakis@chromium.org

[Wez, 2017-01-15 01:29:15]

+thakis: PTAL as OWNER :)

[commit-bot: I haz the power, 2017-01-15 01:56:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-15 01:56:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wez, 2017-01-15 02:20:57]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-15 02:21:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-15 03:04:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-15 03:04:29]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Nico, 2017-01-17 13:03:41]

https://codereview.chromium.org/2288473002/diff/360001/base/allocator/partiti...
File base/allocator/partition_allocator/partition_alloc.h (right):

https://codereview.chromium.org/2288473002/diff/360001/base/allocator/partiti...
base/allocator/partition_allocator/partition_alloc.h:510: CHECK(size + (2 *
kCookieSize) > size);
You lost me in the first file already. Why do we have a CHECK() in a
DCHECK_IS_ON #if? Even if it's intentional for some reason, that looks like a
bug.

[Nico, 2017-01-17 13:06:43]

After looking at the whole patch: Why do we need a new log level for this?
logging.h is too complicated already, and I think we've shipped a dump-on-dcheck
dev channel before without needing this. So I'd rather not add more stuff (and
yet another logging level) if it's not absolutely necessary.

[Wez, 2017-01-17 19:23:30]

On 2017/01/17 13:06:43, Nico wrote:
> After looking at the whole patch: Why do we need a new log level for this?
> logging.h is too complicated already, and I think we've shipped a
dump-on-dcheck
> dev channel before without needing this. So I'd rather not add more stuff (and
> yet another logging level) if it's not absolutely necessary.

Re the complexity of base/logging.h: Yes, it's too complex, and we should fix
that; working through this patch, it appears that there is quite a bit of
complexity in logging.h that could be removed entirely (e.g. unused macros, ones
with duplicate semantics, etc).

Re the extra log level: The original patch provided alternative implementations
for DCHECK & DCHECK_OP, which had a number of limitations, the most fundamental
of which was the disparity between the "meaning" of the LOG_DCHECK/DFATAL level
and the function it actually performs - if a call-site uses DCHECK/DCHECK_OP
then it will dump-on-DCHECK, but if it uses LOG_IF(DFATAL, ...) then it will
crash. We could leave LOG_DCHECK/DFATAL set to LOG_ERROR in dump-on-DCHECK
builds, but then LOG_IS_ON(DCHECK/FATAL) will return false.  Although a new
LogSeverity is a significant change to make, it seemed the cleanest approach,
avoiding a load more base/logging.h complexity.

I'll upload a patch-set using the pure macro-based approach (i.e. no new
LogSeverity) so you can see what that'll look like.

[Wez, 2017-01-27 20:20:30]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 20:21:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 20:36:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 20:36:16]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Kevin M, 2017-01-31 19:41:38]

kmarshall@chromium.org changed reviewers:
- kmarshall@chromium.org

[esprehn, 2017-01-31 20:50:58]

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
File base/allocator/partition_allocator/partition_alloc.h (right):

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
base/allocator/partition_allocator/partition_alloc.h:510: CHECK(size + (2 *
kCookieSize) > size);
This makes PA slower by putting a CHECK inside here. Why is that needed?

[danakj, 2017-01-31 21:45:42]

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
File base/allocator/partition_allocator/partition_alloc.h (right):

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
base/allocator/partition_allocator/partition_alloc.h:510: CHECK(size + (2 *
kCookieSize) > size);
On 2017/01/31 20:50:58, esprehn wrote:
> This makes PA slower by putting a CHECK inside here. Why is that needed?

It's inside DCHECK_IS_ON() so this should be equal to DCHECK() but also it's a
bit odd to write it this way then.

[Will Harris, 2017-01-31 21:53:16]

wfh@chromium.org changed reviewers:
- wfh@google.com

[Wez, 2017-02-06 22:33:20]

Description was changed from

==========
Implement Dump-on-DCHECK, based on dump_on_first_dcheck build argument.

This patch adds a new LogSeverity value, LOG_DUMP, available in all builds.
LOG_DUMP has similar semantics to LOG_FATAL, except that it uploads a dump
without actually crashing, and only does so on the first DUMP-severity failure.

A new DCHECK_IS_DUMP_WITHOUT_CRASHING macro is introduced, which modifies the
behaviour of the LOG_DFATAL/LOG_DCHECK severity in DCHECK_IS_ON()[1] builds,
such that these severities now map to:

  !DCHECK_IS_ON() (e.g. Release)                   -> LOG_ERROR
  DCHECK_IS_ON()                                   -> LOG_FATAL
  DCHECK_IS_ON() + DCHECK_IS_DUMP_WITHOUT_CRASHING -> LOG_DUMP

A new build argument, dump_on_first_dcheck, is set in the GN configuration to
enable the DCHECK_IS_DUMP_WITHOUT_CRASHING macro.

All non-DCHECK_IS_ON() behaviours, e.g. CHECK, PCHECK, SECURITY_CHECK, are
unchanged and continue to be mapped to the LOG_FATAL severity.

Dump-on-DCHECK will be enabled in on-off Canary builds by temporarily landing
patches to set dump_on_first_dcheck in specific configurations.  This underlying
LOG_DUMP implementation is intended to remain checked-in indefinitely.

BUG=596231

[1] DCHECK_IS_ON() is set in Debug builds, and Release builds with
DCHECK_ALWAYS_ON.
==========

to

==========
Implement Dump-on-DCHECK (via a new LogSeverity).

This patch adds a new LogSeverity value, LOG_DUMP, available in all builds.
LOG_DUMP has similar semantics to LOG_FATAL, except that it uploads a dump
without actually crashing, and only does so on the first DUMP-severity failure.

A new DCHECK_IS_DUMP_WITHOUT_CRASHING macro is introduced, which modifies the
behaviour of the LOG_DFATAL/LOG_DCHECK severity in DCHECK_IS_ON()[1] builds,
such that these severities now map to:

  !DCHECK_IS_ON() (e.g. Release)                   -> LOG_ERROR
  DCHECK_IS_ON()                                   -> LOG_FATAL
  DCHECK_IS_ON() + DCHECK_IS_DUMP_WITHOUT_CRASHING -> LOG_DUMP

A new build argument, dump_on_first_dcheck, is set in the GN configuration to
enable the DCHECK_IS_DUMP_WITHOUT_CRASHING macro.

All non-DCHECK_IS_ON() behaviours, e.g. CHECK, PCHECK, SECURITY_CHECK, are
unchanged and continue to be mapped to the LOG_FATAL severity.

Dump-on-DCHECK will be enabled in on-off Canary builds by temporarily landing
patches to set dump_on_first_dcheck in specific configurations.  This underlying
LOG_DUMP implementation is intended to remain checked-in indefinitely.

BUG=596231

[1] DCHECK_IS_ON() is set in Debug builds, and Release builds with
DCHECK_ALWAYS_ON.
==========

[Wez, 2017-02-06 23:42:41]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 23:43:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-02-06 23:50:07]

Have updated the CL description and implemention in
https://codereview.chromium.org/1884023002/ - will finish things up there and
close this alternative implementation out for now.

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
File base/allocator/partition_allocator/partition_alloc.h (right):

https://codereview.chromium.org/2288473002/diff/380001/base/allocator/partiti...
base/allocator/partition_allocator/partition_alloc.h:510: CHECK(size + (2 *
kCookieSize) > size);
On 2017/01/31 21:45:41, danakj (slow) wrote:
> On 2017/01/31 20:50:58, esprehn wrote:
> > This makes PA slower by putting a CHECK inside here. Why is that needed?
> 
> It's inside DCHECK_IS_ON() so this should be equal to DCHECK() but also it's a
> bit odd to write it this way then.

It is important that allocations fail if the requested |size| is nonsensical.
This particular code path is only executed in DCHECK_IS_ON() builds, in which
allocations' sizes are padded out to provide space for "cookies" - the code
assumes that DCHECK will crash the browser, which is no longer the case of
DCHECK_IS_ON() but dump-on-DCHECK is also enabled.

FWIW I've already filed crbug.com/680657 for this, since PA should really be
doing a coherent sanity-check up-front, not relying on each code path happening
to sanity-check things. Similarly, it probably shouldn't pad allocations except
in actual Debug builds.