Interface for defining compilation outputs in the toolchain

[lilygorsheneva]

A large portion of compile.bzl is dedicated to passing input/output files that are feature or toolchain-specific; while this is not necessarily a problem, adding them one-by-one is a bit tedious (see #772); ideally the toolchain should be able to define output files without editing rules_cc itself.

A generic implementation will need to be able to do the following:

• be defined in a toolchain
• be conditional on some aspect of the feature configuration
• declare one or more files, potentially in relation to other files
• add files to output groups

Discussion of PR/772 seems to lean towards callbacks; I propose the following interface for something that would get called (based on feature configuration) in _create_compile_source_action (e.g. how _maybe_declare_trace_file is called in #772)

def callback(
   ctx,               # rule context object. requires committing to keeping ctx in the cc_common.compile API 
   feature_configuration,
   cc_internal, # an object allowing access to file declaration api. Not a good name for it
) -> {"output_group_name":[files]}

When defining a toolchain, add a map or set of these callbacks to CcToolchainInfo (I'm insufficiently familiar with the rules-based-toolchain API to have a proposal for how this should look).

@keith @13steinj @trybka thoughts?

[keith]

I started looking at this too. I have 2 prototypes that go in a different direction:

generate_linkmap_feature = feature(
        name = "generate_linkmap",
        flag_sets = [
            flag_set(
                actions = [
                    ACTION_NAMES.cpp_link_executable,
                ],
                additional_outputs = [
                    additional_output(
                        extension = "map",
                        output_group = "linkmap",
                    ),
                ],
                flag_groups = [
                    flag_group(
                        flags = [
                            "-Wl,-Map=%{output_execpath}.map" if is_linux else "-Wl,-map,%{output_execpath}.map",
                        ],
                        expand_if_available = "output_execpath",
                    ),
                ],
            ),
        ],
    )

and:

flag_groups = [
    flag_group(
        flags = [
            "-Wl,-Map=%{output}" if is_linux else "-Wl,-map,%{output}",
        ],
        expand_if_available = "output_execpath",
        output = "%{output_execpath}.map",
        output_group = "linkmap",
    ),
],

I can imagine issues with both, but I'm not sure how flexible we want to go either

[keith]

Maybe we don't have to go this far with it, but there are a few use cases in the rules that I think it would be ideal if this feature could solve:

• linkmaps as above, pretty easy since it's just an extension change, right now it potentially comes from some flag, but that's fine as long as that flag continues to just enable the feature
• generating pdb files. Notably this is a pretty simple feature case, the only complexity is the file name is derived with some custom logic _strip_extension(binary) + ".pdb" not a simple extension append
• generating dsym files notably for this case the output is actually a directory, not just a file, and the path is then added to link_variables (but that could probably be removed if it was entirely done in the toolchain). In this case the path is also based on something besides the output binary
• serialized diagnostics files, this is the most similar to the ftime-trace case, but the file name is derived based on the artifact category
• dwo files are probably too complicated because those are later consumed by the dwp action
• gcno files are also probably too complicated

[13steinj]

When I tried setting up dwo/dwp files last year, I failed because I think there were some assumptions about the toolchain being made that were difficult for me to reason about.

To some extent, this feels like what the subrules feature was intended for, rather than simple output groups?

I'm flexible on implementation in general but I think it's key to have

• a decent promotion process (user can contribute theirs that the community finds generally useful, maybe with some changes. Maybe as part of a separate ruleset for a while before consensus is reached)
• collision detection and overrideability. Per group of outputs if something of the same name eventually collides, explicitly let the user choose which one with nothing worse than a CLI flag or boolean passed in.

[13steinj]

Also (very approximately) "compatible with toolchain functionality." E.g. Clang past a certain version? -ftime-trace. lld past a certain version? --time-trace. Extra tool provided (CBA), run that. Externis/gcc-time-trace available? I don't think it's that great, but better than nothing.

it would also be very useful if one could stitch together with the bazel profile. Not counting the fact that I could never get it to work/look right, like what ninjatracing was intended to do.

It's also critical that the enablement/disablement of some of these generated files does not affect caching, if possible, for the main action. E.g. yes ftime-trace produces an extra output, but if I run the same action again without ftime-trace this doesn't affect the primary output so should be fully cached (I don't know if time traces should be cacheable at all. I can see both sides of the argument here. There's system resources/contention that affects these files, or "building half the universe again just to get fresh time traces for the other half" is not ideal. I assume the same is true for serialized diagnostics (I am not too familiar with the usage here).

[lilygorsheneva]

compatible with toolchain functionality

If the toolchain itself is defining it, do we need to take extra steps for compatibility checks?

Can a feature state be volatile treated as a volatile key https://bazel.build/docs/user-manual#workspace-status?

(I don't think that analogy quite works; "feature/file that causes a cache invalidation when enabled but not on a transition from enabled to disabled" feels odd to reason about)

[13steinj]

If the toolchain itself is defining it, do we need to take extra steps for compatibility checks?

Some pieces of functionality (e.g. -ftime-trace) are available across numerous toolchain versions, but other sub-pieces (flags that control granularity, for example) are only available after a certain version. Are those intended to be defined as two separate features? If not, and a toolchain claims "compile time tracing exists," albeit being the same conceptual feature, different toolchains at different versions have different levels of configuration granularity. Sometimes there are version ranges where features / subfeatures are fundmentally broken and that has to be accounted for as well (not compilers, but, there are reasons why https://github.com/bazel-contrib/bazel_features exists).

Can a feature state be volatile treated as a volatile key...

I can't think of any way to directly make it so that a compiler flag change will not have an effect.

I hate the fact that I have come up with this idea, but one can have a feature (or whatever) generate a response file (that's the best doc I can find immediately), pass that to the unused_inputs_list when generating the object file, and have that_action.time-trace.json depend on the same inputs and that response file. E.g.

• feature / config flag that with a transition that changes the generation of files time_trace_flags and time_trace_config_flags, the first references the second (maybe these can be collapsed, I am unsure)
• a response file called ancillary_output_flags is always generated, the content of which depends on what features/config flags are enabled (everything disabled-- empty)
• unconditionally pass @ancillary_output_flags to the compile action CLI
• declare a file (the output json) who's time_trace_config_flags specifies as the output path, make a do_nothing action that depends on the .o action and the relevant ancillary flags.

Caveats:

• not every toolchain's response file format supports nesting / being empty. I am sure there are tricks around the "being empty" part, but not the nesting. Can avoid this by always passing in every ancillary response file to the action that generates the object file.
• I think because the CppCompileActions are still native bazel / internal, the native bazel side needs to be changed to support unused inputs in this manner

Of course, there's the argument to be made that the content of the time trace (timings, duh) depends on the entire parameter list, not just the common parameters and the time trace parameters, in which case, depend on all ancillary_output_flags response files as well, not just the compile action + time trace flag response files.

[lilygorsheneva]

Based on internal discussion, I don't think we want to think about complex caching behaviors or maintaining a common repository of these extra outputs: our goal is to be able to support toolchain-specific outputs without toolchains authors sending us PRs or maintaining repo patches.

Similarly, collisions (in our vision) should be resolved by the toolchain owner (a good error message will help but if two outputs collide that's equivalent to a pair of mutually exclusive toolchain features).

(as much as we like caching, I think it's safe to assume that someone gathering profiling data is willing to rerun their compiles an extra time)

[13steinj]

Alright, as soon as my org approves the OSS-contribution internally I'll push up a draft implementation. Intelligent / asymmetric caching behavior is an add-on, can be removed off-the-top.