This is an issue under platforms on which death-tests use fork(), at present. I can draft a CL to address that, but it would be helpful to know how urgent this is - you've marked this as blocking two bugs that have priorities but no milestones. :P

Since it is not a chromium release, didn't put milestones :)

We want to improve the error resiliency and reduce developer confusion when they look at coverage logs. Sooner the better, but we can wait 1-2 weeks for this and manually keep quarantining bad reports (since we can't distinguish from legit failures vs failures from death tests).

Re #2: No problem, but in general we tend to use milestones as a proxy for "want by this date" in P1/P2 bugs, to help prioritize against other work. :)

Some questions about your requirements for these coverage bots:

1. Do you need the death-test expectations to be run, in order to provide correct coverage information? i.e. Could we just disable death-tests entirely for the coverage bot?

2. If we can't disable them, then do you need the death-test sub-processes to not actually crash, in order for coverage information to be reported?  i.e. Is it sufficient to just fix the LOG_DCHECK default to LOG_FATAL in the death-test sub-process, as we do on other platforms?

3. If we can't disable them, nor let the sub-process crash, is coverage reported if we instead base::Process::TerminateCurrentProcessImmediately()? (i.e. if we _exit() under Linux)?  Or do we actually need to exit "normally" for coverage to be reported?

#3 is important because some of the death-tests in Chrome run code which DCHECKs before executing code that will crash if the DCHECK doesn't fire, so we can't just let the death-test sub-process pass through those checks, in general.

#2 is relevant both for DCHECK and CHECK death-tests; even if we configure DCHECKs to be non-fatal, CHECKs remain fatal so will crash the death-test sub-process, and thereby lose coverage reporting?

1. Yes, since there are too many death tests and we would just show lower coverage which seems wrong.

2 and 3. Yes they should not crash at all. Any crash means we lose coverage information (current limitation of clang code coverage). So, can we just have dcheck work like a print and do nothing else in dcheck_is_configurable.

Max, anything you want to add here.

Re #4: Sorry, I phrase (1) poorly: Are you saying "yes, we need to run them" or "yes, we could disable them"?  I couldn't work out which you mean by "there are too many death tests".

Re #5: What about CHECK death-tests (i.e. EXPECT_DEATH rather than EXPECT_DCHECK_DEATH)?

re c#5, I believe that Abhishek meant that we need to run them.

base::Process::TerminateCurrentProcessImmediately() won't work, as it calls _exit() directly, which terminates program immediately, while code coverage is hooked via atexit handler.


re c#6, we're trying to enable both CHECKs and DCHECKs, so none of them should crash under dcheck_is_configurable flag.

As Max said, it is ideal to make both CHECK and DCHECK not crash. don't know if you would prefer both of these to be controlled by another flag name (since dcheck_is_configurable might be confusing to change behavior of CHECKs).

If you feel the need, we can also do a quick sync meeting to make sure we are on same page.

Re #7 & #8: tl;dr: It isn't going to be as simple as that. :P

As I mentioned previously, in general we can't make CHECK and DCHECK not crash, because we have a number of death-tests which assume that they will crash, otherwise the remaining code in the paths under-test will crash. Those tests will need filtering out of the run, or some mechanism adding to switch the CHECK+DCHECKs, and EXPECT[DCHECK_]DEATH() expectations accordingly.

We could add a means to change whether LOG_FATAL is fatal, or not, and replace all EXPECT_DEATH() instances with an EXPECT_CHECK_DEATH() macro that doesn't EXPECT_DEATH unless LOG_FATAL is fatal. However, I don't relish swapping out all those EXPECT_DEATH() calls.

I'd suggest that a proposal be drafted and circulated to the //base OWNERS to comment upon.

How big is the fraction of death tests which assume that they will crash?

Re #10: That will be hard to measure; you'll probably need to try-run a patch which:

1. Builds with DCHECKs.
2. Sets LOG_FATAL to e.g. LOG_INFO.
3. Hacks EXPECT_DEATH(cmd) to just run |cmd| rather than death-testing.

and then review the fallout.

Replace the Summary on this bug, since it isn't really about Death Tests under dcheck_is_configurable. :)

Another potential approach: if the coverage analyzer emits its data only when we exit(), via an atexit() handler, then can we:

1. Build with both CHECKs and DCHECKs wired to LogMessage (i.e. the Debug-mode behaviour).
2. Set up a ScopedLogAssertHandler during death-test startup.
3. Have the log-assert handler function:
  i. Flush the coverage analyzer results to disk (or wherever).
  ii. Invoke base::Process::TerminateCurrentProcessImmediately(), to exit-without-really-crashing, or just IMMEDIATE_CRASH().

WDYT?

Max is OOO from afternoon, he can reply tmrw.

c#14 sounds promising! Thanks for the suggestion.

Unassigning myself though, as I have a ton of other toolchain / dashboard related issues to work on, while this is a Chromium issue that can be addressed by someone else.

 Issue 864190  has been merged into this issue.

It would be terrific to address this. Wez, maybe you can give a shot to the approach you've proposed in c#14? If not, do you know anyone else who is familiar with that stuff and might be able to help? Thank you!

I'll draft a CL for the approach I described, so we can at assess feasibility, maintenance impact, etc.

Nice! Thank you, Wez! The death-tests represent the majority of the tests we see failing on the code coverage bot ( issue 864190 ).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2031d305a65cbb607c5dae9964873edb8c777bb

commit d2031d305a65cbb607c5dae9964873edb8c777bb
Author: Wez <wez@chromium.org>
Date: Thu Aug 16 23:34:25 2018

Flush coverage statistics in process fast-exit & debug-break paths.

Defines a base::debug::WriteClangCoverageProfile() API which performs
an explicit call to __llvm_profile_dump() in CLANG_COVERAGE builds, with
locking to ensure thread-safety.

This API is invoked in the base::debug::BreakDebugger() and
base::Process::TerminateCurrentProcessImmediately() APIs, immediately
prior to them terminating the process, to mirror the coverage-writing
step that would be performed via an at-exit handler during a normal
process-exit.

This ensures that we get as complete coverage data as possible from
processes which fast-terminate/break (e.g. EXPECT_DEATH() sub-processes,
e.g. browser child processes, etc).

Bug:  849369 
Change-Id: I38262334bb5abf8d5ba40c2c32352b38096905ec
Reviewed-on: https://chromium-review.googlesource.com/1172932
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583882}
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/BUILD.gn
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/debug/debugger_posix.cc
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/debug/debugger_win.cc
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/process/process_fuchsia.cc
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/process/process_posix.cc
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/process/process_win.cc
[add] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/test/clang_coverage.cc
[add] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/base/test/clang_coverage.h
[modify] https://crrev.com/d2031d305a65cbb607c5dae9964873edb8c777bb/content/child/child_thread_impl.cc

Check dashboard for r584178 results to see if they look sane/good.

Here it is: https://chromium-coverage.appspot.com/reports/584178/linux/chromium/src/report.html

The total coverage is slightly lower than for other nearby revisions, but that doesn't mean much, as these numbers usually fluctuate a bit.

The same bot is already generating r584312, so we should have a few reports by Monday to look at.

Re #25: It may be that some death-test code paths have no corresponding non-death test associated with them.  e.g. (non style-guide compliant) constructs like:

if (something) {
  NOTREACHED();
  // do stuff to cope with this condition in Release.
}

would execute the "stuff" under dcheck_is_configurable, but not under dcheck_always_on.

Or perhaps I just messed-up the profile-flushing logic. :P

The NextAction date has arrived: 2018-08-20

Reports with dcheck_always_on are marked by '!!!' below:


REVISION	REPORT	LINE COVERAGE	FUNCTION COVERAGE	REGION COVERAGE	LOGS
584342	Tests and Fuzzers   !!!  72.25% (7131362/9870926)	 75.64% (613150/810664)	 73.44% (9397916/12797316)	Link
584332	Tests and Fuzzers	 72.33% (7139321/9870248)	 75.68% (613506/810633)	 73.53% (9409350/12796658)	Link
584331	Tests and Fuzzers	 72.45% (7150584/9870248)	 75.78% (614262/810633)	 73.57% (9414055/12796658)	Link
584329	Tests and Fuzzers	 72.44% (7149854/9870246)	 75.77% (614242/810633)	 73.56% (9413145/12796658)	Link
584328	Tests and Fuzzers   !!!  72.25% (7131194/9870818)	 75.63% (613098/810656)	 73.44% (9398395/12797036)	Link
584327	Tests and Fuzzers	 72.45% (7150725/9870246)	 75.77% (614249/810633)	 73.57% (9414589/12796658)	Link
584318	Tests and Fuzzers	 72.43% (7149286/9870222)	 75.77% (614225/810629)	 73.56% (9413042/12796643)	Link
584316	Tests and Fuzzers	 72.32% (7137807/9870129)	 75.68% (613444/810627)	 73.53% (9409106/12796569)	Link
584312	Tests and Fuzzers   !!!  72.23% (7130113/9870726)	 75.63% (613064/810653)	 73.43% (9397191/12796961)	Link
584308	Tests and Fuzzers	 72.42% (7148358/9870172)	 75.77% (614186/810630)	 73.56% (9412795/12796559)	Link
584227	Tests and Fuzzers	 72.45% (7149574/9868626)	 75.77% (614086/810471)	 73.57% (9413268/12794746)	Link
584183	Tests and Fuzzers	 72.52% (7155829/9867401)	 75.86% (614748/810359)	 73.63% (9419978/12793246)	Link
584178	Tests and Fuzzers   !!!  72.13% (7117670/9867835)	 75.53% (612058/810371)	 73.38% (9387900/12793459)	Link
584149	Tests and Fuzzers	 72.43% (7145968/9865980)	 75.77% (613956/810260)	 73.56% (9409188/12791738)	Link
584064	Tests and Fuzzers	 72.48% (7149581/9864786)	 75.82% (614240/810179)	 73.57% (9409006/12788563)	Link
583867	Tests and Fuzzers	 72.45% (7144158/9860218)	 75.80% (613807/809741)	 73.55% (9402392/12784095)	Link


So it's probably not a coincidence that numbers are slightly lower for that config, but explanation from c#26 sounds fair. We should take a closer look though at a few particular places and enable that config on all the bots.

Re #28: Can we run dcheck_is_configurable and dcheck_always_on coverage runs on the same revision, and diff the reached-lines - I'd expect that then to show a load of DCHECK-but-handle-in-Release code, which in itself would be good to gain visibility of.

Yes, but I think it would be better to run some specific target manually (and also much easier). What would be a good target running death tests?

wez@, do you happen to know which test would be the most useful for testing death tests coverage?

To be frank, I'm having trouble finding a test which would show a significant difference between dcheck_is_configurable and dcheck_always_on. Any suggestions?

Actually, zucchini_unittests is a nice one. It's relatively small, but has a bunch of failures with dcheck_is_configurable and dcheck_always_on.

After inspecting the report, I confirm that the only difference I see is the code that is used for processing those failed EXPECT_DEATH assertions. I'm switching the bots to use dcheck_always_on=true and updating the documentation.

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/code-coverage/+/c95b67eeaeb38e4cbe902347121a58c6744200f3

commit c95b67eeaeb38e4cbe902347121a58c6744200f3
Author: Max Moroz <mmoroz@google.com>
Date: Fri Aug 31 16:07:53 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5a95277c1481fcd74be21b229fee1901a733602

commit a5a95277c1481fcd74be21b229fee1901a733602
Author: Max Moroz <mmoroz@chromium.org>
Date: Fri Aug 31 16:20:55 2018

[Code Coverage] Clarify the differences between DCHECKs and standard asserts.

Bug:  849369 
Change-Id: Iea7f9882faa1daaa4afa82f39c42776cfc4896ce
Reviewed-on: https://chromium-review.googlesource.com/1199704
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Abhishek Arya <inferno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588052}
[modify] https://crrev.com/a5a95277c1481fcd74be21b229fee1901a733602/docs/code_coverage.md
[modify] https://crrev.com/a5a95277c1481fcd74be21b229fee1901a733602/tools/code_coverage/coverage.py

The bots should pick up the update automatically, closing this for now. Will re-open if anything goes wrong.