Code coverage issue - unreachable code is incorrectly being marked as hit by code coverage tool

[schittir]

llvm-cov code coverage tool incorrectly marks the following code as reachable.
This example illustrates the issue: https://godbolt.org/z/ha8PMGGG9

#include <stdlib.h>
#include <stdio.h>

void good() {
  exit(0);
  puts("never get here"); // Code coverage correctly says this is not reached
}

void inner() {
  exit(0);
}
 
void bad() {
  inner();
  puts("never get here"); // Code coverage says we reach this
}

This commit terminates region when noreturn funtion is hit: 181dfe4

Additional instructions to reproduce this issue in a local workspace:
o clang -c code-cov-issue.c -fprofile-instr-generate -fcoverage-mapping
o clang -o app.exe code-cov-issue.o -fprofile-instr-generate
o ./app.exe
o llvm-profdata merge -o app.profdata *.profraw
o llvm-cov show --instr-profile=app.profdata ./app.exe

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: None (schittir)

Details

llvm-cov code coverage tool incorrectly marks the following code as reachable. This example illustrates the issue: https://godbolt.org/z/qErfYqqnP

#include <stdlib.h>
#include <stdio.h>

void good() {
  exit(0);
  puts("never get here"); // Code coverage correctly says this is not reached
}

void inner() {
  exit(0);
}
 
void bad() {
  inner();
  puts("never get here"); // Code coverage says we reach this
}

This commit terminates region when noreturn funtion is hit: 181dfe4

Additional instructions to reproduce this issue in a local workspace:
o clang -c code-cov-issue.c -fprofile-instr-generate -fcoverage-mapping
o clang -o app.exe code-cov-issue.o -fprofile-instr-generate
o ./app.exe
o llvm-profdata merge -o app.profdata *.profraw
o llvm-cov show --instr-profile=app.profdata ./app.exe

[AaronBallman]

It is possible that this is intentional behavior (IIRC GCC behaves the same way but ICC does not), but I don't have enough expertise in coverage to know.

CC @efriedma-quic @rjmccall as codegen code owners

[efriedma-quic]

As an approximation, code coverage assumes assumes calls return. If we didn't make this assumption, we'd have to insert instrumentation after every call to check whether it actually returned. This is almost always the right tradeoff... although it can be a little misleading if you're not aware of it.

We have a special case for calls which are known to be noreturn, but that only works for functions which explicitly have those markings.

[schittir]

As an approximation, code coverage assumes assumes calls return. If we didn't make this assumption, we'd have to insert instrumentation after every call to check whether it actually returned. This is almost always the right tradeoff... although it can be a little misleading if you're not aware of it.

We have a special case for calls which are known to be noreturn, but that only works for functions which explicitly have those markings.

Thanks, @efriedma-quic! https://godbolt.org/z/MP1x4n8Ms works and while this https://godbolt.org/z/z5djGxGM9 generates an invalid noreturn warning. Is this intentional too?

[efriedma-quic]

"function declared 'noreturn' should not return" means you marked a function definition noreturn, but the compiler can't prove whether it actually returns. You can use __builtin_unreachable() if you're doing that intentionally.

[AaronBallman]

but the compiler can't prove whether it actually returns.

But it doesn't need to, it's UB if it does: https://eel.is/c++draft/dcl.attr.noreturn#2 I believe we have the same semantics for all the forms of spelling noreturn, don't we?

What I think confuses me is that an unconditional noreturn call in a function doesn't propagate the "noreturn" to the caller. I could see some logic for not propagating with code like:

void foo(int x) {
  if (x % rand() == 0)
    exit(0);
}

void bar() {
  foo(rand());
  puts("maybe you get here, maybe you don't");
}

but when every path through the function results in calling a noreturn function, I think it seems reasonable to expect that the called function is treated as noreturn even if it lacks the marking. But perhaps that's a diagnostic rather than a code coverage situation? Because the user isn't told about the dead code, they really have no way to know they forgot to mark the outer function as noreturn as well. So maybe this is a request for adding -Wmissing-noreturn or something? But I thought the optimizer already infers this information, so that makes it feel like this really should be a case covered by coverage: https://godbolt.org/z/ddhnTdcnq

[efriedma-quic]

We do noreturn inference at the IR level, but we currently don't try to do it on the AST. We could, maybe.