-
Notifications
You must be signed in to change notification settings - Fork 20
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
Conversation
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
501d505
to
42b76d8
Compare
@googlebot I signed it! |
There was a problem hiding this 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!
"_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", | ||
), |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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. |
@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. |
Superseded by #212 |
Adds support for
bazel coverage
to gather coverage on fuzz targets replayed on the corpus.