Skip to content

Starlarkify and split coverage features - #26355

Closed
fmeum wants to merge 1 commit into
bazelbuild:masterfrom
fmeum:15627-new-coverage-features
Closed

Starlarkify and split coverage features#26355
fmeum wants to merge 1 commit into
bazelbuild:masterfrom
fmeum:15627-new-coverage-features

Conversation

@fmeum

@fmeum fmeum commented Jun 22, 2025

Copy link
Copy Markdown
Collaborator

Before this change, Bazel only enabled a single coverage feature when building with coverage, which doesn't directly allow toolchains to distinguish between targets that are instrumented for coverage and those which are not. Bazel's default toolchains thus end up instrumenting everything even with a narrow --instrumentation_filter, which is wasteful. Blaze relies on the presence of the gcov_gcno_file variable to avoid this, but that's pretty hacky and undocumented (and also undocumentable).

This change lets Bazel request two new coverage-related features: coverage_enabled is requested whenever coverage is generally enabled (matching coverage), whereas coverage_instrumented is only requested when the current target is instrumented. This provides a clean way for toolchains to instrument only those targets that require it (see bazelbuild/rules_cc#424 for an example of such a toolchain).

To make this change possible, coverage feature computation is moved from Java to Starlark. Along the way, implementation_deps is added to the check for an instrumented dependency, which apparently wasn't updated when this attribute was added.

Work towards #15627
Fixes #21911
Fixes #22132 (comment)

@fmeum
fmeum force-pushed the 15627-new-coverage-features branch 6 times, most recently from 05b850b to e55e136 Compare June 23, 2025 13:17
@fmeum fmeum changed the title Starlarkify coverage features Starlarkify and split coverage features Jun 23, 2025
@fmeum
fmeum marked this pull request as ready for review June 23, 2025 15:41
@fmeum
fmeum requested review from c-mita and removed request for fweikert, gregestren, lberki and trybka June 23, 2025 15:41
@github-actions github-actions Bot added team-Rules-CPP Issues for C++ rules team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Jun 23, 2025
@fmeum

fmeum commented Jun 23, 2025

Copy link
Copy Markdown
Collaborator Author

@c-mita for coverage, @pzembrod for C++, @comius for Starlarkification, I guess :-)

@c-mita

c-mita commented Jul 2, 2025

Copy link
Copy Markdown
Member

There are two cases (that I can immediately think if) where a C++ rule needs to consider coverage when it doesn't match the instrumentation filter.

  1. One of its immediate dependencies is matches the filter. This is so header files are appropriately covered.
  2. When linking, we need to include the appropriate coverage dependencies if any cc_library included in the link was compiled with coverage enabled.

So I think the split of the feature is correct, although I might quibble with the naming; we require coverage flags in the link not because coverage is enabled generally, but because a transitive dependency requires it (although we don't actually check for that at the moment, but we could).

@pzembrod pzembrod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm from a rules_cc point of view

Fwiw, I regret to say that CcStaticCompilationHelper.java and compile.bzl are moving targets atm.

@fmeum

fmeum commented Jul 2, 2025

Copy link
Copy Markdown
Collaborator Author

So I think the split of the feature is correct, although I might quibble with the naming; we require coverage flags in the link not because coverage is enabled generally, but because a transitive dependency requires it (although we don't actually check for that at the moment, but we could).

It's true that we require coverage flags in the link for a different reason that is only approximated by coverage being enabled globally. But I don't think that makes it an issue with naming: the coverage_enabled feature is enabled if and only if coverage is enabled globally. Toolchains are free to use this as a stand-in for "transitive dep may be instrumented for coverage".

We could avoid introducing the coverage_enabled feature by adding a different feature that tracks "transitive dep matches the coverage filter", but that again introduces assumptions about the relation between "matched by the filter" and "actually instrumented by the compiler". My opinion is that the potential savings are not worth the extra complexity.

@fmeum
fmeum force-pushed the 15627-new-coverage-features branch 2 times, most recently from d5735bc to 12bf932 Compare July 2, 2025 13:33
@fmeum

fmeum commented Jul 2, 2025

Copy link
Copy Markdown
Collaborator Author

@pzembrod I resolved the conflict.

# Conflicts:
#	site/en/docs/cc-toolchain-config-reference.md
@fmeum
fmeum force-pushed the 15627-new-coverage-features branch from 278d82b to 2849f21 Compare July 15, 2025 09:41
@fmeum

fmeum commented Jul 15, 2025

Copy link
Copy Markdown
Collaborator Author

@c-mita Friendly ping

@fmeum

fmeum commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Moved to bazelbuild/rules_cc#842

@fmeum fmeum closed this Aug 11, 2026
@github-actions github-actions Bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 11, 2026
fmeum added a commit to fmeum/rules_cc that referenced this pull request Aug 12, 2026
…ented`

`cc_common.configure_features` only requested a single `coverage` feature when building with coverage, which doesn't allow toolchains to distinguish between targets that are instrumented for coverage and those that aren't. The default toolchains thus end up instrumenting everything even with a narrow `--instrumentation_filter`, which is wasteful.

`coverage_enabled` is now requested whenever coverage is generally enabled (matching `coverage`), whereas `coverage_instrumented` is only requested if the current target is instrumented. This gives toolchains a clean way to only instrument the targets that require it, see bazelbuild#424 for an example.

Along the way, `implementation_deps` is added to the check for an instrumented dependency, which apparently wasn't updated when this attribute was added.

This is a port of bazelbuild/bazel#26355, with the coverage feature computation kept in `configure_features` instead of moved into the individual rules: unlike Bazel's Java implementation, the Starlark one has access to `ctx`, so every rule calling `cc_common.configure_features` benefits without losing the existing `coverage` and `*_coverage_map_format` features.
fmeum added a commit to fmeum/rules_cc that referenced this pull request Aug 17, 2026
…ented`

`cc_common.configure_features` only requested a single `coverage` feature when building with coverage, which doesn't allow toolchains to distinguish between targets that are instrumented for coverage and those that aren't. The default toolchains thus end up instrumenting everything even with a narrow `--instrumentation_filter`, which is wasteful.

`coverage_enabled` is now requested whenever coverage is generally enabled (matching `coverage`), whereas `coverage_instrumented` is only requested if the current target is instrumented. This gives toolchains a clean way to only instrument the targets that require it, see bazelbuild#424 for an example.

Along the way, `implementation_deps` is added to the check for an instrumented dependency, which apparently wasn't updated when this attribute was added.

This is a port of bazelbuild/bazel#26355, with the coverage feature computation kept in `configure_features` instead of moved into the individual rules: unlike Bazel's Java implementation, the Starlark one has access to `ctx`, so every rule calling `cc_common.configure_features` benefits without losing the existing `coverage` and `*_coverage_map_format` features.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-Documentation Documentation improvements that cannot be directly linked to other team labels team-Rules-CPP Issues for C++ rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation on how to get c++ toolchain to work with bazel converage

3 participants