Fix cargo kcov crashing/generating incorrect reports for proc-macros

[roypat]

Signed-off-by: Patrick Roy [email protected]

Changes

Introduces a hack that forces every invocation of rustc from within the tests/test_coverage.py integration test to include the -Clink-dead-code option, which is required for gathering meaningful coverage reports

Reason

When cross-compiling, cargo does not pass the value of the RUSTFLAGS environment variable to rustc invocations for build scripts, proc-macros and plugins: https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags. The second case here is a problem, as we will (most likely) start introducing procedural macros into our workspace via upcoming PRs. On these PRs, the CI has been either crashing (due to a bug in rustc relating to incorrect debug information being generated for proc-macro crates compiled without -Clink-dead-code option), or generating incorrect coverage reports (since due to the way proc-macro crates are built, uncovered code gets eliminated during linking, meaning it will not show up as "uncovered" in the resulting report (this is kcov specific behavior)).

Alternatives considered

1. We can work around cargo's handling of the --target flag in relation to RUSTFLAGS by passing the --target option via RUSTFLAGS. This generally works, but causes problems in seccompiler-bin

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

• All commits in this PR are signed (git commit -s).
• If a specific issue led to this PR, this PR closes the issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this PR.
• New unsafe code is documented.
• API changes follow the Runbook for Firecracker API changes.
• User-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.

---

• This functionality can be added in rust-vmm.

[pb8o]

Did you try using CARGO_TARGET_x86_64_unknown_linux_musl_RUSTFLAGS=-Clink-dead-code as an alternative?

[roypat]

Yes, those flags only affect the code that is actually compiled to run on the specified target, and thus also excludes proc macros and build scripts

[pb8o]

Actually it should be CARGO_TARGET_X86_64_UNKNOWN_LINUX_MUSL_RUSTFLAGS=-Clink-dead-code (uppercased), but if it's not going to apply by design then probably not worth trying again.

[roypat]

I did try both! (if you use the lowercase option then cargo will tell you to use the uppercase one instead)

[bchalios]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

In any case, it is always nice to have a meaningful commit body, describing the problem and the solution

[roypat]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

[bchalios]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

Do we have to define --target when running cargo kcov?

[roypat]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

Do we have to define --target when running cargo kcov?

We can specify a no target, in which case everything would work out-of-the-box. We'd be collecting the coverage results for x86_64-unknown-linux-gnu instead of musl, then.

[bchalios]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

Do we have to define --target when running cargo kcov?

We can not specifiy a target, in which case everything would work out-of-the-box. We'd be collecting the coverage results for x86_64-unknown-linux-gnu instead of musl, then.

Sure, but I guess that's not an issue for us, right? We run the same code, regardless if we target gnu or musl libc.

[roypat]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

Do we have to define --target when running cargo kcov?

We can not specifiy a target, in which case everything would work out-of-the-box. We'd be collecting the coverage results for x86_64-unknown-linux-gnu instead of musl, then.

Sure, but I guess that's not an issue for us, right? We run the same code, regardless if we target gnu or musl libc.

I honestly don't know if it's an issue. In the last meeting it was said that we'd prefer to run the coverage on musl. @mattschlebusch do you have any insight on this?

[alxiord]

In the last meeting it was said that we'd prefer to run the coverage on musl.

I think that was because the binaries we publish in releases are musl-built, but indeed the coverage we care about is run strictly on firecracker code irrespective or architecture.

[mattschlebusch]

Yes, it is quite hacky. Were you not able to find another way to pass the correct combination of flags?

Sadly no, the only way to set codegen options is either to use (one of the variations of) 'RUSTFLAGS' (which cargo ignores for proc-macro crates if a target is set (either via --target, or with CARGO_BUILD_TARGET environment variable, or via target directive in .cargo/config), or to hijack rustc somehow. The canonical way to do this would be via RUSTC_WRAPPER, but since cargo kcov parses the rustc output to determine the location of the binary to run code coverage on, this doesn't work. So this was the only thing I could think of.

Do we have to define --target when running cargo kcov?

We can not specifiy a target, in which case everything would work out-of-the-box. We'd be collecting the coverage results for x86_64-unknown-linux-gnu instead of musl, then.

Sure, but I guess that's not an issue for us, right? We run the same code, regardless if we target gnu or musl libc.

I honestly don't know if it's an issue. In the last meeting it was said that we'd prefer to run the coverage on musl. @mattschlebusch do you have any insight on this?

I will defer to those with more code coverage tooling experience. The wildcard for me here is that generated code like macros built by proc_macro2 could be different depending on the build target. But I don't believe this should ever be true. In which case using the *-gnu target should be logically sufficient albeit a bit less legible.

[roypat]

What we could do is collect code coverage for all non-proc-macro crates on *-musl and only run the proc-macro crates using *-gnu. Since I think we cross-compile our release binaries (?), the proc-macros are compiled for the gnu target in production anyway.

This would require some minor changes to the test script (instead of invoking cargo kcov --all we would have to loop over all crates in our workspace and then only pass --target=x86_64-unknown-linux-musl to those that are not proc-macros, and then merging the runs using kcov --merge) and to .cargo/config to remove the global target directive. I think we always pass --target to all our cargo invocations anyway (although this would have to be checked ofc).

[roypat]

Closing as we switched to grcov in #3173