xd009642 / tarpaulin

A code coverage tool for Rust projects
https://crates.io/crates/cargo-tarpaulin
Apache License 2.0
2.5k stars 180 forks source link

No coverage for cdylib crate called from C/C++ executables #1156

Closed Luthaf closed 1 year ago

Luthaf commented 1 year ago

Describe the bug

I have a project (https://github.com/Luthaf/rascaline/, coverage branch, PR with CI is https://github.com/Luthaf/rascaline/pull/126) which is built as both a rust crate (tarpaulin works great here, thanks a lot!) and exports a C API through a cdylib crate (rascaline-c-api). The C API is then used to create a C++ (and Python) interface for the code.

The C and C++ API are tested as a normal C/C++ project, using CMake to first build the rust code, and then build C and C++ test executables. I made sure to pass the right flags to rustc (setting RUSTFLAGS="-Cdebuginfo=2 --cfg=tarpaulin -Cinstrument-coverage -Clink-dead-code") when building the rust shared library.

The LLVM-based coverage instrumentation seems to work fine, generating .profraw files (I added a small patch in https://github.com/Luthaf/tarpaulin/commit/c67eb17884510e06fe7fca0ce61fc15805367537 since the profraw files are generated inside a different directory), but then no coverage is reported for the c-api crate at all. I added a handful of dbg! inside cargo-tarpaulin and llvm-profparser, and as far as I understand the code it seems that the profraw files contain at least some coverage data.

To Reproduce

git clone https://github.com/Luthaf/rascaline
cd rascaline 
git checkout coverage
cargo tarpaulin --all-features --workspace --engine=llvm --test=c-api-tests --skip-clean --verbose

Here is the output I get: tarpaulin-rascaline.txt

I'm wondering if there could be an issue with file name parsing or something like this, since all the path shown are relative to the rascaline-c-api crate, and not the root of the repository/root of the cargo workspace.

If you have a couple of pointer/thinks I could look for, I'm happy to continue debugging this and send a PR once it is fixed.

Expected behavior

Using an instrumented shared library from C or C++ should be able to collect and report coverage on the C API implementation in rust.

xd009642 commented 1 year ago

One potential for debugging is using the CLI tools I did in https://github.com/xd009642/llvm-profparser to see how the profraw files and binary look in terms of paths etc (or the relevant llvm tools).

Luthaf commented 1 year ago

Ok, I think I found the issue!

Using the actual test executable as an object file makes the code miss a lot of executed functions:

../llvm-profparser/target/release/cov show --instr-profile target/tarpaulin/profraws/tests/c-api/c_api_tests-468943f98a4ac45c_16259556021706463993_0-2295843.profraw --object target/debug/deps/c_api_tests-468943f98a4ac45c

But by forcing the code to use the shared library as an object file, it is able to report coverage for the expected functions

../llvm-profparser/target/release/cov show --instr-profile target/tarpaulin/profraws/tests/c-api/c_api_tests-468943f98a4ac45c_16259556021706463993_0-2295843.profraw --object target/debug/librascaline.so

I guess this makes sense since the test executable never loads this shared library. Instead it execute ctest, which then executes the actual C/C++ executables, which load this shared library.


I guess the simplest thing to do might be to use the tools from llvm-profparser manually to extract coverage information, and use that in CI.

Do you think it would make sense to add an option to cargo tarpaulin to manually add other object files to the list that ends up loaded in the CoverageMapping? https://github.com/xd009642/tarpaulin/blob/57f2a75ea2b85334e6f0980ea5f0b84475c15698/src/statemachine/instrumented.rs#L134

xd009642 commented 1 year ago

sorry for late response, I do think that feature makes sense and I'd be happy to add it or accept a PR to do so :smile:

EDIT: working on it now, so no need for a PR

xd009642 commented 1 year ago

Okay I've implemented this and I can see it works (output at bottom). I'll just be making a PR and merging it once everything is shown to pass

All the increases in coverage show difference before me doing it and now with the new --objects arg:

Command ran: cargo tarpaulin --all-features --workspace --engine=llvm --test=c-api-tests --skip-clean --objects target/debug/librascaline.so

|| Tested/Total Lines:
|| rascaline/src/calculator.rs: 73/113 +64.60%
|| rascaline/src/calculators/descriptors_by_systems.rs: 106/118 +89.83%
|| rascaline/src/calculators/dummy_calculator.rs: 38/41
|| rascaline/src/calculators/lode/radial_integral/gto.rs: 0/58
|| rascaline/src/calculators/lode/radial_integral/mod.rs: 0/33
|| rascaline/src/calculators/lode/radial_integral/spline.rs: 0/9
|| rascaline/src/calculators/lode/spherical_expansion.rs: 0/384
|| rascaline/src/calculators/neighbor_list.rs: 0/211
|| rascaline/src/calculators/radial_basis/gto.rs: 37/39
|| rascaline/src/calculators/radial_basis/mod.rs: 2/8
|| rascaline/src/calculators/soap/cutoff.rs: 31/59
|| rascaline/src/calculators/soap/power_spectrum.rs: 213/278 +76.62%
|| rascaline/src/calculators/soap/radial_integral/gto.rs: 24/30
|| rascaline/src/calculators/soap/radial_integral/mod.rs: 28/29
|| rascaline/src/calculators/soap/radial_integral/spline.rs: 7/7
|| rascaline/src/calculators/soap/radial_spectrum.rs: 0/80
|| rascaline/src/calculators/soap/spherical_expansion.rs: 261/326
|| rascaline/src/calculators/sorted_distances.rs: 0/41
|| rascaline/src/errors.rs: 7/34
|| rascaline/src/labels/keys.rs: 52/62
|| rascaline/src/labels/samples/atom_centered.rs: 58/72
|| rascaline/src/labels/samples/long_range.rs: 0/27
|| rascaline/src/labels/samples/mod.rs: 5/6
|| rascaline/src/math/double_regularized_1f1.rs: 57/65 +87.69%
|| rascaline/src/math/eigen.rs: 162/167
|| rascaline/src/math/erf.rs: 0/98
|| rascaline/src/math/exp.rs: 0/40
|| rascaline/src/math/gamma.rs: 19/165
|| rascaline/src/math/hyp1f1.rs: 91/142
|| rascaline/src/math/hyp2f1.rs: 0/170
|| rascaline/src/math/k_vectors.rs: 0/42
|| rascaline/src/math/spherical_harmonics.rs: 100/127 +78.74%
|| rascaline/src/math/splines.rs: 101/115 +87.83%
|| rascaline/src/systems/cell.rs: 23/107
|| rascaline/src/systems/chemfiles.rs: 21/25
|| rascaline/src/systems/neighbors.rs: 117/133
|| rascaline/src/systems/simple_system.rs: 25/32
|| rascaline/src/types/matrix.rs: 36/66 +54.55%
|| rascaline/src/types/mod.rs: 8/36
|| rascaline/src/types/vectors.rs: 12/37 +32.43%
|| rascaline-c-api/src/calculator.rs: 65/72 +90.28%
|| rascaline-c-api/src/logging.rs: 17/19
|| rascaline-c-api/src/profiling.rs: 19/23
|| rascaline-c-api/src/status.rs: 29/37 +78.38%
|| rascaline-c-api/src/system.rs: 121/172 +70.35%
|| rascaline-c-api/src/utils.rs: 10/10
|| rascaline-c-api/tests/utils/mod.rs: 26/28 +1.19%
|| 
50.11% coverage, 2001/3993 lines covered, +44.34% change in coverage
Luthaf commented 1 year ago

Thanks a lot, that's great!!

Luthaf commented 1 year ago

Any reason for the call to canonicalize here: https://github.com/xd009642/tarpaulin/blob/f92c1bb02c43a3415efd33676e220bbcdc1e4f25/src/config/parse.rs#L39

It fails on CI, since the shared library does not exist yet when the Config is created from the ArgMatches.

xd009642 commented 1 year ago

Generally just to get an absolute path, because they're needed in various replaces (reports, coverage collection) and relative paths are pernicious with workspaces and hard to keep track of for normalising at the end. However, that canonicalise is for the --manifest-path arg. I guess you meant this one https://github.com/xd009642/tarpaulin/blob/f92c1bb02c43a3415efd33676e220bbcdc1e4f25/src/config/parse.rs#L129

It might be possible to remove it some how, or maybe moving the join to the end after the unwrap would fix it.

xd009642 commented 1 year ago

@Luthaf if you try out the fix/canonicalise-object branch it might fix the issue for me (let me know please :pray: )

Luthaf commented 1 year ago

Thank you so much for the quick fix! I needed another small change, which I put in #1225. With that, I'm able to collect coverage for my project.

xd009642 commented 1 year ago

Cool I'll look at the PR and merge etc and look to get a release out once I'm done with work today :+1:

xd009642 commented 1 year ago

And this is released now