Make gcno-files available in CcCompilationOutputs

[torgil]

This is needed to do custom code coverage rules.

[googlebot]

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

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[torgil]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[BrunoChevalier]

We also need this functionality added to bazel.
Can we get a confirmation that it will be merged?

[torgil]

We also need this functionality added to bazel.
Can we get a confirmation that it will be merged?

You also may want this to be able to pick the correct files in a clean way: #12229

[laurentlb]

ping @oquenchil?

[thekyz]

Is somebody looking at that?

[oquenchil]

I agree with the goal of this PR and Bazel needs to allow you to do this. However, can you first try out if you can get to these files through the InstrumentedFilesInfo?
https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/starlarkbuildapi/test/InstrumentedFilesInfoApi.java;drc=0afdecfe6fce0aac8704d4d07a1132db2b25759a;l=50

If there is any reason why that's not possible, then the way you do it here makes sense.

[torgil]

Yes. We're doing custom rules using cc_common interface. InstrumentedFilesInfo is not available from there.

For these kind of files it for me looks cleaner if the featue or rule specifying the options generating the files also specifies the additional output files. In this case it's tricky though due to the pic/nopic situation and the fact that the rule/feature has no control over object naming.

[torgil]

We also need this functionality added to bazel.
Can we get a confirmation that it will be merged?

You also may want this to be able to pick the correct files in a clean way: #12229

While waiting for cc_binary to become Starlark we could also use the map file to select correct gcno file (pic vs nopic), see #12883 how to get this file.

[oquenchil]

Yes. We're doing custom rules using cc_common interface. InstrumentedFilesInfo is not available from there.

Why not? Is Bazel not making the API available to you to use InstrumentedFilesInfo? If it's just a matter of taste, I'd go with those custom rules starting to use InstrumentedFilesInfo since that's the mechanism and API that we have.

[torgil]

Yes. We're doing custom rules using cc_common interface. InstrumentedFilesInfo is not available from there.

Why not? Is Bazel not making the API available to you to use InstrumentedFilesInfo? If it's just a matter of taste, I'd go with those custom rules starting to use InstrumentedFilesInfo since that's the mechanism and API that we have.

I'm using cc_common.compile to create the compile action. It returns no InstrumentedFileInfo.

[torgil]

Ping @oquenchil
We have also a need of making the .d - file and the .su - file available to Starlark after compilation for header coverage tests and other tests so I'd like to make this patch more general. I'm thinking in the lines of something similar to the "additional_outputs" that was made for the linker, the problem here is that

1. The output path for these files is not known to starlark in advance making it harder to declare files
2. There are both pic and nopic versions that needs to be added

For 1 maybe if the object paths are stable, paths could be hard-coded in rules. 2 could possibly be solved with two arguments, "additional_outputs" and "additional_pic_outputs".

Any ideas?

[torgil]

I've rebased this patch on master (5.0 prereleases), it required updates for gcno-files to show up using the "build" command.
edit: Github UI seems broken as it doesn't update this PR after rebase/force-push.

Patch on master is here: https://github.com/torgil/bazel/tree/torgil/compile-gcno-output

If you're looking for a patch on 4.1.0, see https://github.com/torgil/bazel/tree/torgil/compile-gcno-output-4.1.0

[torgil]

Rebased versions of coverage patches + additional essential patches can be found here: https://github.com/torgil/bazel/commits/master

[sgowroji]

@torgil, I'm going to go ahead and close this PR, because it seems to have stalled. If you're still interested in pursing this (and responding to my comments), please feel free to reopen! Thank you!

[torgil]

@avielas

https://docs.bazel.build/versions/main/command-line-reference.html#flag--collect_code_coverage

The gcno-files are generated during compilation (not during test) and are already available in CcCompilationOutputs. They are just not accessible from Starlark which is what this patch do.

Bazel test/coverage also runs outside the normal build tree which makes it not useful if the information is needed as input to another higher level target.

[torgil]

@sgowroji Yes. This patch is critical to us in combination with #12229 which solution needs to be revisited. Please reopen.

[torgil]

I first thought that an "additional_outputs" argument to cc_common.compile similar to cc_common.link should be more generic and useful but were discouraged due to the pic/nopic situation. It it possible to have both pic and nopic result from a single cc_compile call?

[torgil]

I talked with someone over the phone at NYC Bazel Con that said an "additional_outputs" option was feasible in conjunction with some way to detect if a binary was linked with pic or nopic. That would be a more generic solution to this problem and would cover all kinds of files (analog to "additional_outputs" in cc_common.link) like stack usage files and dependency files (for which we currently have another custom patch). Is anyone following up on this?

[oquenchil]

Hey torgil that was me. I will prioritize this for after the holiday break. Will be working together with nikhilkalige who filed this issue: #15924

Completely different problem but I think we can arrive at a versatile solution that allows people to get different files and flags through for their specific use cases without needing to pipe them via Bazel.

After the break I will describe what the solution could look like, will group many issues together like this one that could benefit from it and nikhilkalige will take it on from there.

[oquenchil]

Created #17237

[oquenchil]

After looking at this pull request again and the issue that I created. I'm thinking now that it doesn't really fit. The issue here is for gcno files whose output artifact is already created by Bazel, this is about piping them through CcCompilationOutputs to custom coverage rules. I didn't see the mismatch until having them side by side. I don't want to delay you any longer, if you rebase this change I will merge it.

[torgil]

Thanks. Awesome. I have a few followup questions:

1. How should the custom rule choose which gcda-files (pic or nopic) that matches a gcda-file after the executable has been run?
   Remember that I currently have a patch on the linking outputs for this.
2. Will the general case be addressed in another topic /PR?

[oquenchil]

Is there a compiler flag that lets you customize the suffix of the gcda file? If that's the case we can accept a PR that adds support for this in Bazel and the default toolchain. The code would add the suffix *.pic.gcda for .pic.o.

If not, I need to know more details about how the custom coverage rule is implemented. Does it depend directly on a cc_binary? How are you getting to the object files, is it with an aspect?

If it depends directly on the cc_binary, what you can do is to have an aspect (that doesn't propagate through attributes, just one level) on the custom coverage rule applied to the cc_binary dependency. The aspect can look at the target's actions, get to the linking action and get the list of inputs, here you would build a list of paths from the inputs (all of it in analysis). Later when iterating over all the objects in analysis as you were doing, you can check if its path is on the list (or dictionary) that you built.

Related that it might be of interest to you is that in the past month internally we decided to just build everything as PIC when not in production. See commit 973f6258c6, the mock toolchain shows the feature that you'd have to add to your toolchain or to the default Bazel one which doesn't have it yet.

If this is only for coverage you may as well just build everything with PIC.

[torgil]

The code would add the suffix *.pic.gcda for .pic.o.

This starts to get rather complex as it needs modifications to

1. cc_common to add parameters for gcda-file suffix
2. testrunner action to clear gcda-files (non-sanbox platforms) and detect/rename the produced one (to match declare_file) and save the suffix state in another file (to return pic/nopic state for input to the coverage action)
3. coverage action needs all gcno-files, the gcda-file and the pic/nopic state file

Maybe both step 1 and step 2 could be removed if the coverage action get access to the map-file from the binary linking step in which it can see object file paths and match it with gcno file paths. Still quite complex compared to accessing pic/nopic state in LinkingOutputs.

Does it depend directly on a cc_binary?

It has access to the output of cc_common.link

How are you getting to the object files, is it with an aspect?

The outputs of cc_common.compile is fed to cc_common.link through CcCompilationOutputs (which this PR wants more information from)

get to the linking action and get the list of inputs

Can this be done directly after the call to cc_common.link in the rule producing the binary as well ?

If this is only for coverage you may as well just build everything with PIC.

I will not always have control over all toolchains (downstream) so I prefer not to add that dependency.

[Wyverald]

could you rebase your change? it looks like some of the fields you're adding are already present

[sgowroji]

Hi @torgil, Above PR is awaiting to merge. Could you please do necessary rebase changes as mentioned above.Thanks!

[torgil]

could you rebase your change? it looks like some of the fields you're adding are already present

Rebase fails as this functionality already implemented behind a private API.

commit c449a8211039f9145c111daa4697721ecbe18dec
Author: Googler <cmita@google.com>
Date:   Tue Dec 13 07:09:26 2022 -0800

    Internal change
    
    PiperOrigin-RevId: 495012726
    Change-Id: Ib02f5c6f844fa42b57e249dc37cd0f413626c8e0

I remade the patch to remove the private api check instead. I'm not sure how to do with the "enableCoverage" check, I just removed it for now as it's more tided to "bazel coverage" command than the "coverage" feature. With current patch It'll generate an empty gcno file if coverage feature is not enabled which may not be optimal.

[oquenchil]

With current patch It'll generate an empty gcno file if coverage feature is not enabled which may not be optimal.

We can't have this. I understood this PR to just be a refactoring to allow you to get to the files. Why not leave the enableCoverage check? It shouldn't affect whether you are able to get to the files or not. This PR shouldn't cause any behavioral changes.

I remade the patch to remove the private api check instead.

I agree with this but now that it's public API, could you please add a test to StarlarkCcCommonTest?

Reviewed old code. Currently discussing new changes.

[phst]

Any updates on this?