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

C++ toolchain callbacks #17237

Open
oquenchil opened this issue Jan 18, 2023 · 9 comments
Open

C++ toolchain callbacks #17237

oquenchil opened this issue Jan 18, 2023 · 9 comments
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@oquenchil
Copy link
Contributor

oquenchil commented Jan 18, 2023

Description

Add the option to create a Starlark method tied to the C++ toolchain callable before actions

There are many feature requests that involve adding code to the Bazel C++ rules in order to pipe the variables necessary for very specific use cases:

  • a new output by the compilation or linking actions not yet contemplated by the rules

  • flags that have to be generated dynamically and therefore cannot go in copts or linkopts but for which there isn’t an existing build variable

It would scale better if the C++ rules code base didn’t have to add explicit support for each of these use cases.

Work

The community will drive adding support for this feature, it first requires discussion in a design doc and reviewing existing issues labeled team-rules-Cpp in bazelbuild/bazel to make sure that we consider every use case that could benefit from this. Once the design is approved, the author can proceed with implementation.

Issues that could benefit (important to find more to justify the work here):

  1. Custom build variable with short filename: Additional SOURCE_FILENAME variable in CppCompileVariables #15924

  2. AOSP issue for linking which can benefit from this: CppLink actions having access to fdo_profile_path variable #17277

Possible implementation

(This is an idea of how I imagine it roughly, final implementation should be guided by the design.)

Placement

Currently the C++ rules are structured as follows:

Compilation

  1. Rule outer later (cc_library, cc_binary, etc..)

  2. CcCompilationHelper.java (we will start rewriting to Starlark this quarter)

  3. CppCompileActionBuilder

  4. CppCompileAction

Linking

  1. Rule outer later (cc_library, cc_binary, etc..)

  2. CcLinkingHelper

  3. CppLinkActoinBuilder

  4. CppLinkAction

The toolchain callback should probably go between 2 and 3, in other words, it should be called once for every CppCompileAction and every CppLinkAction created. This allows adding the custom output per action.

Possible callback signature for CppCompileAction

def toolchain_callback(ctx, source): # is cc_toolchain and feature config needed  here?
        # Logic specific to each implementation goes here
        # …..
	# Output which would then be read from the C++ rule’s implementation and passed
        # when constructing the CppCompile action
	return {
                 “additional_inputs” : new_inputs,
                 “additional_outputs” : new_outputs,            
                 “extensions” : extensions, # Build variable extensions for the crosstool
            }

The extensions would be a simple dictionary like the one passed here.

For #15924, the flags can be created in a loop one by one based on the source passed.

There are plans for Q1 and Q2 to starlarkify CcCompilationHelper but meanwhile the part of the C++ rules logic that would invoke the callback is still in Java despite the toolchain callback being in Starlark. Calling Starlark code from Java is possible via Starlark.call (see here, here and here).

The compilation callback would be stored in the CcToolchainProvider and called before creating every CppCompilation action. Similarly for the linking callback. The callback can be passed as an argument here. This feature request only makes sense for projects that are using Bazel's C++ rules and want to add customization on top, a project with custom rules doesn't need this feature.

The suggestions above can be modified completely after experimenting with real use cases. After the design discussion and experimenting contributors may also conclude that this isn't really needed and there are better ways to achieve the same thing.

@oquenchil oquenchil added type: feature request P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules help wanted Someone outside the Bazel team could own this labels Jan 18, 2023
@oquenchil
Copy link
Contributor Author

@nikhilkal would you still be interested in driving this?

@brentleyjones
Copy link
Contributor

If this existed, would #15191 have used this capability?

@oquenchil
Copy link
Contributor Author

oquenchil commented Jan 19, 2023

So all of this is kind of subjective. First, we should consider how widely useful something is to decide whether to embed logic for something like that in Bazel or have it purely in the toolchain with the feature described here. If it's generally useful, then we shouldn't force everyone to implement it in their own toolchain. Do the same flags work in gcc or MSVC?

I don't really have a sense of how often people are using that diagnostics feature you added. If we could have the code that supports it living in the toolchain in Starlark and not annoy anyone, then why not?

At first glance, it looks like it is indeed the type of flag that the feature requested here could help with. As an exercise the design for this feature could try to satisfy that use case even if the PR is already merged, just to prove that it's flexible enough.

We can consider later whether we should rewrite that PR using this feature.

@brentleyjones
Copy link
Contributor

Do the same flags work in gcc or MSVC?

No, I believe it's limited to clang.

@vinhdaitran
Copy link

#17277 describes the issue in AOSP for linking.

@nikhilkalige
Copy link
Contributor

@oquenchil I pinged you on slack.. How can I help

@oquenchil
Copy link
Contributor Author

As much as you can afford in driving this issue from design to implementation. I linked 2 use cases (including yours) in the original description then #15191 is another issue that would have benefited from this.

If the design satisfies those 3 use cases, that'd show enough flexibility, if you find even more issues related to the C++ rules that would benefit from this, even better. Next week I'm planning to start looking at every issue (about 400) and see if I can find common themes. I will keep an eye for any that could be solved with this.

Meanwhile I think you could start a design doc for the 3 examples that we have. Here you can find many examples of proposals: https://github.com/bazelbuild/proposals

@daivinhtran
Copy link
Contributor

@oquenchil has there been any progress in this work?

@oquenchil
Copy link
Contributor Author

No, we are looking for contributions here as described in the issue description and my previous comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Someone outside the Bazel team could own this P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

No branches or pull requests

5 participants