[coverage] nested macro with unused branch leads to incorrect coverage

[justincady]

Code coverage reporting is incorrect at the call site when a macro contains an unused branch. Example:

// macro.c
#include <stdio.h>

int enabled = 0;

#define MY_ID(x) 7

#define MY_LOG(fmt, ...)            \
  {                                 \
    if (enabled) {                  \
      printf(fmt, ## __VA_ARGS__);  \
    }                               \
  }

int main(int argc, char *argv[]) {
  MY_LOG("%d, %s, %d\n",
         MY_ID(argc),
         "a",
         1);
}

# build-c.sh
/usr/bin/clang --version | /bin/grep "clang version"
/usr/bin/clang -fprofile-instr-generate -fcoverage-mapping coverage.c -o coverage
./coverage
/usr/bin/llvm-profdata merge -sparse default.profraw -o default.profdata
/usr/bin/llvm-cov show ./coverage -instr-profile=default.profdata

The coverage report confusingly marks the MY_LOG usage in main as partially covered:

$ ./build-c.sh
clang version 19.1.7
    1|       |#include <stdio.h>
    2|       |
    3|       |int enabled = 0;
    4|       |
    5|      0|#define MY_ID(x) 7
    6|       |
    7|       |#define MY_LOG(fmt, ...)            \
    8|      1|  {                                 \
    9|      1|    if (enabled) {                  \
   10|      0|      printf(fmt, ## __VA_ARGS__);  \
   11|      0|    }                               \
   12|      1|  }
   13|       |
   14|      1|int main(int argc, char *argv[]) {
   15|      1|  MY_LOG("%d, %s, %d\n",
   16|      1|         MY_ID(argc),
   17|      0|         "a", // INCORRECT
   18|      0|         1);  // INCORRECT
   19|      1|}

If the example is updated to avoid using MY_ID withing MY_LOG, the problem disappears (even if the updated line has a branch):

$ ./build-c.sh
clang version 19.1.7
    1|       |#include <stdio.h>
    2|       |
    3|       |int enabled = 0;
    4|       |
    5|       |#define MY_ID(x) 7
    6|       |
    7|       |#define MY_LOG(fmt, ...)            \
    8|      1|  {                                 \
    9|      1|    if (enabled) {                  \
   10|      0|      printf(fmt, ## __VA_ARGS__);  \
   11|      0|    }                               \
   12|      1|  }
   13|       |
   14|      1|int main(int argc, char *argv[]) {
   15|      1|  MY_LOG("%d, %s, %d\n",
   16|      1|         enabled ? 1 : 2, // Now the entire MY_LOG usage is covered
   17|      1|         "a",
   18|      1|         1);
   19|      1|}

I suspect this is specific to macro expansion because I can substitute other constructs, including function calls, in the place of MY_ID and get correct results.

Also, I believe the ideal behavior here is that all lines within MY_LOG usage in main should be covered; we did execute that line and that's what usually happens. Then, appropriately, the MY_LOG implementation lines should be marked uncovered.

[justincady]

I think I've changed my opinion on what the ideal behavior is here.

I now think the MY_ID(argc) line should be marked uncovered and is not:

   14|      1|int main(int argc, char *argv[]) {
   15|      1|  MY_LOG("%d, %s, %d\n",
   16|      1|         MY_ID(argc), // INCORRECT: this argument was never evaluated
   17|      0|         "a",
   18|      0|         1);
   19|      1|}

Why the change?

Ideally, we want the most accurate coverage possible. In this case, the most accurate report possible would indicate no argument was actually evaluated. If MY_ID was holding a potential bug of some kind, our report should reflect whether or not it was executed prior to shipping.

Note that instead of a macro a MY_ID function, called here, would naturally require evaluation of all arguments prior to its execution.

I spent some time trying to understand why the MY_ID line execution count is 1. It appears the actual mapped counter value is correct (0), but llvm-cov reports the line as 1 because the MY_ID segment is wrapped by another segment (see: llvm/lib/ProfileData/Coverage/CoverageMapping.cpp). Specifically: the segment beginning at 16:10 is wrapped by the segment beginning at 15:9.

I don't yet understand if that indicates a bug in the segment wrapping processing versus being a known limitation.

Another note: this behavior only occurs with __VA_ARGS__. If I convert to a parameterized macro, (e.g. MY_LOG(fmt, a, b, c)) all lines of main are covered. One could argue we should maintain consistency, but again my current opinion is we should strive for the additional accuracy when we can.

cc @ZequanWu @chapuni

[chapuni]

@justincady Could you take a look into #119282?

[justincady]

@chapuni Thank you for the link. I read through it and I applied #120842, #121191, and #121192 locally but unfortunately my issue described above still remains.

Do you think those PRs are foundational to resolving the issue here?

[justincady]

@chapuni After further investigation I think I understand your direction to that issue. With #120842, #121191, #121192, and #121194 you would provide the options for merging counters differently. Even with those patches though, each merge strategy of #121194 yields the same result for my example.

But I do think that the counter merging philosophy is the root cause of my example. If I enable the existing debug logging in CoverageMapping.cpp:

Combined regions:
  5:18 -> 5:19 (count=0)
  8:3 -> 12:4 (count=1)
  9:9 -> 9:16 (count=1)
  9:18 -> 11:6 (count=0)
  10:22 -> 10:33 (count=0)
  14:34 -> 19:2 (count=1)  <--------------------
  15:3 -> 15:9 (count=1)
  16:10 -> 18:11 (count=0) <--------------------
  16:10 -> 16:15 (count=0)
[...]
Segment at 14:34 (count = 1), RegionEntry <--------------------
Segment at 15:3 (count = 1), RegionEntry
Segment at 15:9 (count = 1)
Segment at 16:10 (count = 0), RegionEntry <--------------------
Segment at 16:15 (count = 0)
Segment at 18:11 (count = 1)
Segment at 19:2 (count = 0), Skipped

And here is where the line count is computed in CoverageMapping.cpp:

  // Pick the max count from the non-gap, region entry segments and the
  // wrapped count.
  if (WrappedSegment)
    ExecutionCount = WrappedSegment->Count;
  if (!MinRegionCount)
    return;
  for (const auto *LS : LineSegments)
    if (isStartOfRegion(LS))
      ExecutionCount = std::max(ExecutionCount, LS->Count);

We see the line execution count is the max of the wrapped segment count (1) and the line segment count (0). And that leads to the confusing report.

[justincady]

I posted a draft PR with a proposed (and maybe incomplete) direction to resolve this. I'm not sure if it's the right way to go (and it will reduce existing coverage). As detailed above, working on this led me to philosophical debates about what the line coverage should report here. :)

It's possible that keeping what exists today (without my PR) is the better way forward.