Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Luthaf opened this issue Dec 6, 2022 · 11 comments
Closed

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

Luthaf opened this issue Dec 6, 2022 · 11 comments
Assignees

Comments

@Luthaf
Copy link
Contributor

Luthaf commented Dec 6, 2022

Describe the bug

I have a project (https://github.com/Luthaf/rascaline/, coverage branch, PR with CI is Luthaf/rascaline#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 Luthaf@c67eb17 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
Copy link
Owner

xd009642 commented Dec 6, 2022

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
Copy link
Contributor Author

Luthaf commented Dec 6, 2022

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?

CoverageMapping::new(&binaries, &instrumentation).map_err(|e| {

@xd009642
Copy link
Owner

xd009642 commented Jan 26, 2023

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 😄

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

@xd009642
Copy link
Owner

xd009642 commented Jan 29, 2023

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
Copy link
Contributor Author

Luthaf commented Jan 29, 2023

Thanks a lot, that's great!!

@Luthaf
Copy link
Contributor Author

Luthaf commented Feb 21, 2023

Any reason for the call to canonicalize here:

.canonicalize()

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

@xd009642
Copy link
Owner

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

.canonicalize()

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

@xd009642
Copy link
Owner

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

@Luthaf
Copy link
Contributor Author

Luthaf commented Feb 24, 2023

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
Copy link
Owner

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

@xd009642
Copy link
Owner

And this is released now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants