DCHECKs with custom string outputs also look to be counted as lines that aren't reached.

Thanks mmenke@, could you please kindly provide a link to an example of DCHECKS with custom string outputs?

For the DCHECKS, we may want to explore the "dcheck_always_on" config.

The above link also has such a DCHECK (See line 519).  Or if you want one with the DCHECK closer to the top, you can look at https://chromium-coverage.appspot.com/reports/558266/linux/chromium/src/net/base/escape.cc.html.

Thanks!!!

Thanks Matt! I agree that would be useful, let's keep thinking about it. Unfortunately, I don't see any potential solution at this point, as the coverage instrumentation happens on the source code level, even before any optimizations are done (it's actually an advantage of clang code coverage), so even a stupid example like the following:

$ cat test.c 
int main() {
  int a = 0;
  return a;

  return 0;
}

$ cat build_and_run.sh 
#!/bin/bash
clang -fprofile-instr-generate -fcoverage-mapping test.c -o test
LLVM_PROFILE_FILE="1.profraw" ./test
llvm-profdata merge -sparse *.profraw -o dump.profdata
llvm-cov show -instr-profile=dump.profdata ./test


Will mark "return 0;" as non-covered.

I think I've seen something similar reported upstream, I'll try to find it.

Deleted: Selection_366.png 10.4 KB

	Selection_366.png 10.4 KB View Download

Another example from dalecurtis@:

"
NOTREACHED(), CHECK(), DLOG() all seem to count against region coverage:

https://chromium-coverage.appspot.com/reports/558266/linux/chromium/src/media/ffmpeg/ffmpeg_decoding_loop.cc.html
"

One option would be to just define DCHECKS and the like to be whitespace.  Another option would be to do post-processing (Ignoring lines in the same block as a NOTREACHEDs might be hard in that case, but at least ignoring DCHECKs and the like shouldn't be too hard, depending on the output format).

Making DCHECKs whitespace will likely trigger compiler warnings - hadn't thought of that.  Yaks, all the way down.

https://bugs.llvm.org/show_bug.cgi?id=36086 seems relevant

there also was another issue with macros which should be fixed now: https://bugs.llvm.org/show_bug.cgi?id=34237

I'm not sure that I agree with the use case for this. If you have something completely unreachable, you won't put any code for that. But if you put something like UNREACHABLE assert, that means you give it a little chance to happen (in case of an error, cosmic rays, etc), and you want to be asserted when it happens. Since this is the case, that means you'll be pleased to see that all UNREACHABLE blocks are red, i.e. you never fall into those unexpected situations.

Take a look at the example from c#0 [1], there is a comment that clearly explains why that code technically can be reached. That means, we shouldn't try to remove these lines from the report.

Would it be possible to re-define these macro as no-op when #if defined(CLANG_COVERAGE)?

[1]: https://chromium-coverage.appspot.com/reports/558266/linux/chromium/src/net/url_request/url_request_job.cc.html#L213

I guess I should've explained why I'm suggesting to re-define the macro. I believe that if it doesn't produce any executable code, it will be treated as a comment, and thus it will stick to some neighbor code region and show it's coverage. I'm not sure how it will look in case of empty void functions like from the first example, but in other cases I'd expect it to look better and not to have that red highlight if all the code around is covered.

We can't do dcheck_always_on=true since both tests and fuzzers need the same gn flags. enabling dchecks will lead to fuzzers crashing instantly and we losing coverage. if we dont have same gn flags, then fucntion code differ, so coverage data can't merged.

I think some sort of redefining macro, as Max cited in C#14, sounds like the only solution.

NOTEACHED() are assertions that code can't be reached.  I'd think that wide code coverage, you'd want to verify that code your care about is actually covered.

I'm not going to waste my time scrolling through every file in net/ to make sure every line is a NOTREACHED() or is covered - that's just not a productive use of my time.  If numbers less than 100% of coverage were actually meaningful, it may in fact be useful for do that.

> NOTEACHED() are assertions that code can't be reached

exactly, and if that code is not covered, it wasn't reached indeed. If a developer can be 100% sure that something is unreacheable, they won't write a code for that, right?

I understand what you want, and I agree that might be a very handy extra feature, like an extra column or an extra button "Coverage minus regions manually marked unreachable", but that doesn't feel right to be a default representation, as the one and only purpose of those NOTREACHED() macros is to alert when something surprisingly is reached. If it wasn't true, that macro wouldn't be written.

I many be 100% sure code can't be reached, but put it in anyways.  NOTREACHED() also serves both as documentation and as protection against regression. - I'd argue that every branch that can't be reached should have a NOTREACHED in it (Or the block shouldn't exist in the first place - if it's an if that always triggers, for instance, then the if generally isn't needed, and should be a DCHECK instead).

> NOTREACHED() also serves both as documentation and as protection against regression.

Right, and that protection is only possible because there might be at a chance that the code can be reached under some very special circumstances. This is why we can't strip those lines out by default :( But we still can try to have some post-processing for easier evaluation of the stats.

The simplest solution to try, though, would be to re-define the macro when "#if defined(CLANG_COVERAGE)" and see how much it improves the numbers and the reports. 

I understand the point about accuracy, but here it feels much more annoyance.

Lets try to explore the re-defining macro point.

Also, for overall picture, it is important for dev/teams/tpms to know the overall count without these negatives for dchecks/dlog/notreached, etc.

We already have hooks for the old Clang static analysis (see base/logging.h) to hint to it to assume e.g. that when processing code after DCHECK(condition), then |condition| must be true; can we use something similar to annotate the debug info in the generated code with not-reachable so we can use that to exclude those lines from reports?

Abhishek has reminded me one important thing in an offline discussion. If a process crashes, we don't get coverage from it. That somewhat invalidates my point regarding NOTREACHED() code being actually reachable. It still can be reachable though, but we'll never know when that happens. Removing those regions from the reports now makes more sense to me.

Re c#22, I'm skeptical that that approach will work, because coverage instrumentation is applied very early before the complication. I think we either have to:

A) make it no-op or even comment out using preprocessor

B) do some post-processing after generating the reports

I'd suggest exploring A) in the mean time, because I can't promise B) in this quarter -- that depends on another huge chunk of work for on-the-fly report generation, which is not even in progress as of now.

As for DCHECKs, Abhishek suggested a nice idea to enable those for coverage build in a non-crashing mode, so we'll get all of them covered, but won't lose any coverage as we won't crash.

Re #24: We already have a flag to allow DCHECKs to be built-in and the |condition|s verified, but without crashing the process - see |dcheck_is_configurable|.  Note that some DCHECKs conditions will lead to the subsequent code crashing anyway, if they're not met, though.

Thanks, i think i can atleast add dcheck_is_configurable to help us a little bit.

Let me explain with example of where it works and where it does not work. Lets say size is size_t.

DCHECK(size >= 0) << "Uncovered" << size;
if (size < 0) {
  NOTREACHED();
}
DLOG(ERROR) << "Uncovered" << size;

in this code 
entire DLOG line is covered - nice :)
NOTREACHED is never called - so it will still show red.
since size >= 0, we see red on "(size >= 0) << "Uncovered" << size;" since never executed.

So, in a way we need to find something to completely neuter / make DLOG/DCHECK/NOTREACHED a no-op.

I see
#define EAT_STREAM_PARAMETERS \
  true ? (void)0              \
       : ::logging::LogMessageVoidify() & (*::logging::g_swallow_stream)

Is there a nice way to make this no-op :) ?

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

commit 6fffcd495d96950e494c144ceae95b9fc5714986
Author: Abhishek Arya <inferno@chromium.org>
Date: Thu May 17 15:15:43 2018

I wonder if the coverage instrumentation could use hints like e.g. https://cs.chromium.org/chromium/src/base/logging.h?q=dcheck&sq=package:chromium&dr=CSs&l=305 to ignore lines for coverage.

NOTREACHED() otoh is harder since it has pretty fuzzy semantics. It doesn't always mean "this isn't never reached, and if it is that's a bug", we sometimes just use it as "not (yet) implemented".

I would love if reaching a NOTREACHED was considered undefined behavior since we could then we could define it to __builtin_unreachable() / __assume(0) which would help with warnings and codegen and what have you, but that has semantics that are pretty different from what we have used NOTREACHED for in many years, so I'm afraid changing that might not be feasible. (It would help with this bug too though.)

Re #28: Re hints, yes, that's what I was referring to in comment 22. :)

We have a separate NOTIMPLEMENTED() so hopefully folks don't use NOTREACHED() when they really mean not-implemented.

Would it help to have NOTREACHED() compile to something with a noreturn annotation (akin to e.g. TerminateCurrentProcess)? (Or perhaps it does already..)

Re 29: https://cs.chromium.org/search/?q=NOTREACHED.*TODO&sq=package:chromium&type=cs The fuchsia/crashpad ones for example look like "not implemented"s...

Change in C#27 is reverted due to  bug 844432 .
Can't find any way of annotations currently in clang coverage.

Quick fix might be some way to make EAT_STREAM_PARAMETERS as no-op without compile errors.
We will file upstream bug and see what can be done about annotations.

I'd be okay with NOTREACHED() code that isn't reached (e.g. not even by a death test) be marked as uncovered.

What's bugging me right now is that (D)CHECKs that are clearly run (and pass) are also marked as uncovered, e.g. https://chromium-coverage.appspot.com/reports/560344/linux/chromium/src/base/task_scheduler/lazy_task_runner.cc.html

So is code that strictly runs on DCHECK_IS_ON() builds, e.g. SchedulerLockImpl : https://chromium-coverage.appspot.com/reports/560344/linux/chromium/src/base/task_scheduler/scheduler_lock_impl.cc.html

Can we also run coverage from a dcheck_always_on build?

gab@, i will turn on dchecks using dcheck_is_configurable=true, once blocker  bug  845505  is resolved tmrw. I tried it in c#27 and it caused breakages, so that blocker bug needs to be fixed before it can be reenabled.

Re #36: Do you need |dcheck_is_configurable| or would |dcheck_always_on| suffice?  The problem with |dcheck_is_configurable| is that death-tests don't currently work correctly under POSIX platforms (ones that launch death-test sub-processes via fork()) when |dcheck_is_configurable| is set.

We can't use dcheck_always_on since it will cause a hard crash during fuzzing (fuzzing triggers random data). So, it will hit a lot in fuzzing builds (which share the same build config as regular tests otherwise coverage data can't be merged).
dcheck_is_configurable gives us the options to cover those one-line DLOG, DCHECK (without the << part) by printing them as warnings.

Re #38: Won't the code-under-coverage crash out in some otherwise hard-to-fathom way if fuzzing causes DCHECKs to be violated, anyway, though?

Will you be running any DCHECK-death-tests under this setup?

We use release builds and crash-free corpus for fuzzers, so they do not crash otherwise.

Side-thought : fuzzers easily being able to hit DCHECKs seems like a problem. Should we fuzz for DCHECKs (orthogonal to code coverage but would help this issue).

re c#41, that's a good point, and we use debug builds for fuzzing as well (https://build.chromium.org/deprecated/chromium.fyi/builders/Libfuzzer%20Upload%20Linux%20ASan%20Debug), we just don't use that configuration for coverage generation stuff.

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

commit 35a89260c5816a61492c23378e6990752479ec6c
Author: Abhishek Arya <inferno@chromium.org>
Date: Thu May 24 15:09:15 2018

DCHECK, DLOG, etc now show up, e.g. see https://chromium-coverage.appspot.com/reports/561573/linux/chromium/src/net/base/escape.cc.html#L20

Sometimes, we see a red on just a DCHECK_{X}E macro name itself, we have still no clues on why that occurs. If anyone has time to debug and get this fixed, that would be great. https://chromium-coverage.appspot.com/reports/561573/linux/chromium/src/net/base/escape.cc.html#L21

Re #44: DCHECK_OP is a multi-line macro with paths for success and failure, so some lines will not be reached unless you actually trigger failure, e.g. with a death test.

DCHECK_LE(i, 15) << i << " not a hex value";

Does red on DCHECK_LE means that this DCHECK never failed ? I was a little confused on that since the part "<< i << " not a hex value";" didn't turn red.

That looks kinda similar to https://bugs.llvm.org/show_bug.cgi?id=34237.


Issue 843223 can also somehow affect this, but it's hard to say for sure.

Re #48: The DCHECK-enabled implementation lives at https://cs.chromium.org/chromium/src/base/logging.h?l=903

In your example DCHECK_LE() will resolve to a LogMessage instance, or to nullptr. Either way the items appended to the log macro with operator<< will be evaluated, but the LogMessage line will only be reached on failure.