Add a coverage-diff.py script

[tlively]

Add a (vibe coded) script for collecting LLVM code coverage reports and intersecting them with git diffs to produce coverage reports for the current change. The script can be used by both LLMs and humans to ensure that a change has proper test coverage in a deterministic, trustworthy manner.

Update the lit config so that the config at the project root does not override the config in a particular output directory. This ensures that when the new script runs lit tests with the coverage build in a separate build directory, it uses the binaries from the coverage build, not the normal in-tree build.

Also update the early exit logic to do a normal exit when collecting coverage. This ensures that the cleanup handlers run and the coverage is actually written to the coverage file.

[tlively]

I read through the script and it all seemed pretty reasonable, but I didn't do an in-depth review, and I don't recommend anyone else spend time doing an in-depth review of it, either.

[kripken]

Perhaps worth adding some documentation about how to use this? Does it need a special build or version of LLVM? Also an example of the output would help the reader understand the motivation here.

[tlively]

Yes, it needs a special build, but it detects when that build is missing and gives usage instructions:

Error: Build is not configured for coverage reporting.
  Reason: Build in '/usr/local/google/home/tlively/code/binaryen' uses compiler '/usr/bin/c++' instead of Clang (Clang is required for source-based coverage mapping).

To collect multi-metric diff coverage, configure a Clang coverage build:

  1. Configure CMake with Clang source-based coverage flags (e.g. in 'out/cov'):
     cmake -S . -B out/cov -G Ninja \
       -DCMAKE_BUILD_TYPE=Debug \
       -DCMAKE_C_COMPILER=clang \
       -DCMAKE_CXX_COMPILER=clang++ \
       -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc" \
       -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping -fcoverage-mcdc"

  2. Build the targets exercised by your tests (e.g. wasm-opt, binaryen-lit, binaryen-unittests):
     ninja -C out/cov wasm-opt binaryen-lit binaryen-unittests

  3. Run this tool with your test(s) to collect profiles and report diff coverage in one step:
     ./scripts/coverage-diff.py -B out/cov --lit test/lit/passes/<your-test>.wast
     ./scripts/coverage-diff.py -B out/cov --gtest "*YourTest*"
     ./scripts/coverage-diff.py -B out/cov --run "out/cov/bin/wasm-opt ..."

Here's the output from #9126 (as an arbitrary example) with --annotate:

=== Diff Coverage Report (uncommitted changes vs HEAD) ===

  Metric       Covered / Total        Details
  ------------ ---------------------- ----------------------------------
  Lines        6 / 6 (100.0%)         Executable lines in diff
  Regions      7 / 7 (100.0%)         Sub-line / block AST regions in diff
  Branches     7 / 8 (87.5%)          True/False branch outcomes in diff
  MC/DC        2 / 3 (66.7%)          Condition independence pairs in diff
  Functions    2 / 2 (100.0%)         Modified functions called >= 1x

--- Per-File Summary ---
  src/passes/MergeSimilarFunctions.cpp
    Lines: 6 / 6 (100.0%), Regions: 7 / 7 (100.0%), Branches: 7 / 8 (87.5%), MC/DC: 2 / 3 (66.7%)
  src/wasm/wasm-validator.cpp: (only comments / non-executable lines changed in diff)

=== Uncovered / Partially Covered Code in Diff ===

File: src/passes/MergeSimilarFunctions.cpp
  Uncovered / One-Sided Branches:
    - Line 258:11-33 `lhsCallee != rhsCallee` -> missing FALSE branch (True: 8x, False: 0x)
  Uncovered MC/DC Condition Independence Pairs:
    - Lines 258:11-261:63 `lhsCallee != rhsCallee && ...` -> missing independence pair for C1

=== Annotated Diff Hunks ===

--- src/passes/MergeSimilarFunctions.cpp ---
@@ lines 80-84 @@
     80 |         | #include "ir/module-utils.h"
     81 |         | #include "ir/names.h"
+    82 |         | #include "ir/public-type-validator.h"
     83 |         | #include "ir/utils.h"
     84 |         | #include "opt-utils.h"
@@ lines 250-265 @@
    250 |      2x |         return false;
    251 |      2x |       }
+   252 |         |       // Parameterizing direct calls to different functions requires creating
+   253 |         |       // `ref.func` and `call_ref` instructions for them. In an open world, this
+   254 |         |       // can cause a previously private function signature to become public (for
+   255 |         |       // instance, if `funcref` is publicly exposed). Do not parameterize the
+   256 |         |       // call if the callee's signature is not a valid public type (e.g., if it
+   257 |         |       // contains an exact reference when custom descriptors are disabled).
+   258 |      8x |       if (lhsCallee != rhsCallee &&
        |         |   ^-- Branch (258:11): [True: 8x, False: 0x]
+   259 |      8x |           getPassOptions().worldMode == WorldMode::Open &&
+   260 |      8x |           !PublicTypeValidator(module->features)
+   261 |      6x |              .isValidPublicType(lhsCallee->type.getHeapType())) {
+   262 |      1x |         return false;
+   263 |      1x |       }
    264 |         |
    265 |         |       // Arguments operands should be also equivalent ignoring constants.

--- src/wasm/wasm-validator.cpp ---
@@ lines 251-258 @@
    251 |   5327x |   }
    252 |         |
+   253 |         |   // TODO: This only checks directly exposed root types. To catch all invalid
+   254 |         |   // public exact references (such as types reachable from exposed types or
+   255 |         |   // subtypes of exposed `funcref` in open-world mode), we should check all
+   256 |         |   // public heap types if we can do so without making validation too expensive.
    257 |   2832x |   for (auto& [type, _] : ModuleUtils::getExposedPublicHeapTypes(module)) {
    258 |   2832x |     for (auto child : type.getTypeChildren()) {

I'll add brief usage information to this comment.

[kripken]

I see, thanks. I feel that more could be in the new comment, though? Here is how I look at it: when someone looks at this script, it is useful for the docs to explain a bit what it actually does, without them needing to run it first.

Specifically, how about part of the output of the Diff coverage report? That would give a clear motivation I think.

[kripken]

(I mean part of the Diff coverage report that you pasted here)

[tlively]

I do want to keep the doc comment high-level to avoid duplication and the possibility of it getting stale. I'm also assuming that the user either wants coverage information or not, and having extra detail in this comment won't make much of a difference.

[kripken]

But I don't feel strongly, lgtm if you don't want to add more to the comment.

One other suggestion, though: how about putting this in scripts/experimental, where "experimental" means "not thoroughly validated/not fully recommended by the project" - ?

[tlively]

Sure, I can put it in a scripts/experimental directory. Do you have some criteria in mind for graduating it out of experimental status?

[kripken]

Full, careful human review?

[tlively]

Ok, I think that probably makes sense to do, but we can get some experience with how useful this ends up being first.