bazel
bazel copied to clipboard
bazelbuild
instrumentation_filter doesn't control what files to be instrumented
Description of the bug:
When code coverage is enabled using --collect_code_coverage=True, no matter if a file matches the instrumentation_filter it will always be compiled with instrumentation flags turned on. isCodeCoverageEnabled in CcCimpilationHelper should be used to guard it in CcCommon.
What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.
Run an aquery with instrumentation_filter and --collect_code_coverage=True on a target and verify that "-fprofile-instr-generate" and "-fcoverage-mapping" are always present no matter if the target match the instrumentation_filter or not.
Which operating system are you running Bazel on?
linux
What is the output of bazel info release?
development version
If bazel info release returns development version or (@non-git), tell us how you built Bazel.
git clone https://github.com/bazelbuild/bazel.git git checkout 3791b049d3db4c87dd0c02c3de2f52686fced0eb the run bazel build //src:bazel
What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?
git remote get-url origin: https://github.com/bazelbuild/bazel.git
git rev-parse master: 128e4b32d6279fd76d403d37be2a2d485a51acc1
git rev-parse HEAD: 3791b049d3db4c87dd0c02c3de2f52686fced0eb
Have you found anything relevant by searching the web?
No response
Any other information, logs, or outputs that you want to share?
No response
Triaging to P3 with some reservation. Optimising to only instrument filtered file sounds fine. API modifications should be conservative (I didn't look into details on how this could/should be done). Accepting PRs depends on proposed API changes being accepted first.
cc @fmeum As was mentioned in the discussion for PR #15634.
Simply changing CcCommon::configureFeaturesOrThrowEvalException to check CcCompilationHelper::isCoverageEnabled(RuleContext) isn't sufficient.
((https://cs.opensource.google/bazel/bazel/+/fe1c1768:src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java;l=982)
The problem is that test rules, which are often filtered out by the ``--instrumentation_filter`, may need to link C++ code that has been generated by dependencies. If those dependencies are compiled with coverage information, then the test rule needs to link with coverage support enabled.
Currently the C++ rules would avoid this situation to a degree because CcCompilationHelper::isCoverageEnabled(RuleContext) also checks the status of targets in deps (https://cs.opensource.google/bazel/bazel/+/fe1c1768:src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java;l=2137).
However, this does not help custom rules that include dependencies via a different attribute (one could imagine a rule with a cdeps attribute for C++ dependencies), and it may not even be sufficient in the general case for C++ (From my quick reading of the code, cc_test(foo_test) -> cc_library(foo) -> cc_library(bar) might fail to link if only bar is matched by the instrumentation filter).
One way to fix this may be to add "coverage" to the set of features that can be passed to cc_common.configure_features; that way Starlark rules have control over when to enable it (enabling it for "compile" calls when the current target is set to be instrumented, and for "linking" calls when coverage is generally enabled).
We could also hide these checks within the cc_common.compile/link calls, so rule authors don't have to worry about it and making mistakes.
@oquenchil, do you have any opinions w.r.t. C++ API?
How about splitting the coverage feature into one for compilation and one for linking? The one for compilation could be guarded by the instrumentation filter matching the target, whereas the one for linking would be activated unconditionally.
