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

Add coverage support for fuzzing_regression_test #174

Closed
wants to merge 1 commit into from

Conversation

zhenyudg
Copy link
Contributor

Adds support for bazel coverage to gather coverage on fuzz targets replayed on the corpus.

@google-cla
Copy link

google-cla bot commented Jul 30, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

fuzzing/private/regression.bzl Outdated Show resolved Hide resolved
.bazelrc Show resolved Hide resolved
@zhenyudg
Copy link
Contributor Author

zhenyudg commented Aug 4, 2021

@googlebot I signed it!

Copy link
Collaborator

@stefanbucur stefanbucur left a comment

Choose a reason for hiding this comment

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

Apologies for the delay!

.bazelrc Show resolved Hide resolved
fuzzing/private/regression.bzl Outdated Show resolved Hide resolved
Comment on lines +71 to +80
"_lcov_merger": attr.label(
default = "@bazel_tools//tools/test:lcov_merger",
executable = True,
cfg = "host",
),
"_collect_cc_coverage": attr.label(
default = "@bazel_tools//tools/test:collect_cc_coverage",
executable = True,
cfg = "host",
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I need to understand this better: Why do we need to add these dependencies? Are these going to be built even in non-coverage mode?

(It would help if you could point me to some documentation discussing this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

collect_cc_coverage runs the gcov/llvm-cov commands to gather coverage.

lcov_merger merges different coverage output files into a single output file for consumption.

I found these dependencies from the implementation of the native cc_test rule.

They will be built in non-coverage mode. Native rules with coverage support use late-bound attributes to avoid building these dependencies unnecessarily, but that's not yet an option in Starlark.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately I'm reluctant to accept this change as-is. I understand this is a Starlark limitation, but the workaround of including these dependencies on all builds is inadequate IMO.

Let me do some additional research to better understand all of our alternatives. If coverage support is not mature enough yet in Bazel, I would postpone the addition of this feature for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be fixed by bazelbuild/bazel#13983. Until then, I don't know of a better solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying issue is now a release blocker for Bazel 5, so it's likely that lcov_merger will be provided by default in a way that it would be built only with bazel coverage. The same does not apply to collect_cc_coverage, but that is merely a shell script and thus causes no compilation overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

My fix will not be accepted, so this will very likely not be possible with Bazel 5.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zhenyudg I haven't forgotten about this. If you want to know whether there has been progress in getting the underlying issue resolved in Bazel, you can track bazelbuild/bazel#8736.

Copy link
Contributor

@fmeum fmeum Feb 4, 2022

Choose a reason for hiding this comment

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

@zhenyudg While working on the upstream fix, I may have found a workaround for Bazel 4+: Use an alias as the default for _lcov_merger and in that alias, use a select on a config_setting for collect_code_coverage to switch between the real lcov_merger target and a dummy target (e.g., an empty executable file). Let me know if you need help with this.

@fmeum
Copy link
Contributor

fmeum commented Sep 26, 2022

@zhenyudg Are you still planning to work on this PR? I have a concrete need for this feature now and would thus like to pick it up soon.

@zhenyudg
Copy link
Contributor Author

@fmeum Pardon the radio silence on the MR. I'm on leave until October 3, and I would be more than happy if you can pick this up.

@zhenyudg
Copy link
Contributor Author

zhenyudg commented Oct 6, 2022

Superseded by #212

@zhenyudg zhenyudg closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants