Skip to content

Add Clang -ftime-trace support to rules_cc - #772

Open
PikachuHyA wants to merge 1 commit into
bazelbuild:mainfrom
PikachuHyA:upstream_bazel_trace_support
Open

Add Clang -ftime-trace support to rules_cc#772
PikachuHyA wants to merge 1 commit into
bazelbuild:mainfrom
PikachuHyA:upstream_bazel_trace_support

Conversation

@PikachuHyA

Copy link
Copy Markdown
Contributor

Declare trace JSON files as compile action outputs and expose them via the trace_files output group, enabled with features = ["trace"] on Clang toolchains.

Fix bazelbuild/bazel#9047

@PikachuHyA

Copy link
Copy Markdown
Contributor Author

i think the failed case is not related with this PR.

https://buildkite.com/bazel/rules-cc/builds/5543/canvas?sid=019f030e-cfd4-495f-888e-9f123c072e07&tab=output

@fmeum could you take a look?

@fmeum

fmeum commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

I don't know why this happens. @trybka

Declare trace JSON files as compile action outputs and expose them via the trace_files output group, enabled with features = ["trace"] on Clang toolchains.
@PikachuHyA
PikachuHyA force-pushed the upstream_bazel_trace_support branch from 4d3c124 to 9b9465d Compare June 29, 2026 12:44
@lilygorsheneva

Copy link
Copy Markdown
Collaborator

Slowki in bazelbuild/bazel#9047 mentions using an aspect to collect this (as does an internal project), this makes me unsure whether it needs to be in the rules (though it should be @trybka or @pzembrod 's call)

@PikachuHyA

Copy link
Copy Markdown
Contributor Author

Slowki in bazelbuild/bazel#9047 mentions using an aspect to collect this (as does an internal project), this makes me unsure whether it needs to be in the rules (though it should be @trybka or @pzembrod 's call)

@lilygorsheneva Thanks for flagging this
I considered the aspect-based approach before but don't think it's a viable full replacement, for two reasons:

1. An aspect would be fragile for C++20 Modules. The current implementation in _create_cc_compile_actions_with_cpp20_module is not a flat "one source → one object" flow. rules_cc creates c++-module-deps-scanning actions, aggregates .ddi files into modules info, generates per-source .modmap / .modmap.input files, distinguishes module interface sources from normal compilation units, wires direct/transitive .pcm module files, and passes module-specific build variables into _create_compile_source_action. An external aspect would have to duplicate that internal wiring to correctly instrument module builds, and would be very easy to break as the modules pipeline evolves.

2. It also pushes a compatibility burden onto users. Bazel 9 moves C++ compilation onto rules_cc's own Starlark internals, so an aspect written for the pre-9 native C++ rules may need additional adaptation for Bazel 9. Implementing this inside rules_cc keeps the compatibility story simpler: as long as the user's rules_cc version is compatible with their Bazel version, trace collection should work normally. With an external aspect, users may need to handle version-specific differences themselves, so it is less likely to work out of the box across Bazel / rules_cc versions.

For example, to make bazelbuild/bazel#9047 (comment) work for bazel 9, the patch needed.

diff --git a/MODULE.bazel b/MODULE.bazel
new file mode 100644
index 0000000..133dd0f
--- /dev/null
+++ b/MODULE.bazel
@@ -0,0 +1,4 @@
+module(name = "clang_aspects")
+
+bazel_dep(name = "rules_cc", version = "0.2.17")
+bazel_dep(name = "bazel_skylib", version = "1.9.0")
diff --git a/clang_time_trace.bzl b/clang_time_trace.bzl
index b88be59..a6db9e8 100644
--- a/clang_time_trace.bzl
+++ b/clang_time_trace.bzl
@@ -1,6 +1,8 @@
 load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@rules_cc//cc:action_names.bzl", "ACTION_NAMES")
 load("@rules_cc//cc:find_cc_toolchain.bzl", "find_cc_toolchain", "use_cc_toolchain")
+load("@rules_cc//cc/common:cc_common.bzl", "cc_common")
+load("@rules_cc//cc/common:cc_info.bzl", "CcInfo")

 def _clang_time_trace_aspect_impl(target, ctx):
     # Get the compile commands

Given that -ftime-trace needs to be attached to the actual compile actions constructed by rules_cc, I think keeping this in rules_cc is less fragile than requiring users to maintain an external aspect that mirrors those internals. Happy to defer to @trybka / @pzembrod on the final call.

@trybka trybka left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't understand the Android failures but they certainly seem unrelated. @keith any thoughts?

Re: aspect vs. not

We currently use an aspect for this internally ourselves.
I don't know that we want to go down the path of adding bespoke logic for every compiler/linker feature that emits another file. (One could imagine adding link support for --cref -Map, for instance).

I wonder if there's a better way to define a feature with a generic additional output file that gets fed in with a build variable, then we don't need the rules to handle every permutation, just a magic feature with a specific build variable.

That might require that they have unique names though.


_VALID_CPP_SOURCE_TYPES = set([CPP_SOURCE_TYPE_SOURCE, CPP_SOURCE_TYPE_HEADER, CPP_SOURCE_TYPE_CLIF_INPUT_PROTO])

def _clang_trace_enabled(feature_configuration):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is ftime-trace clang specific?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

AFAIK yes, there's a GCC plugin that gives some advanced metrics but I believe it's kinda defunct.

https://github.com/royjacobson/externis

"lto_compilation_context": {},
"gcno_files": [],
"pic_gcno_files": [],
"trace_files": [],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we need to track the .pic files separately, or could we bundle them together?

@keith

keith commented Jul 14, 2026

Copy link
Copy Markdown
Member

android stuff is fixed now so we can ignore

@keith

keith commented Jul 14, 2026

Copy link
Copy Markdown
Member

IMO we should support someway for toolchains to add files like this in a generic way. we have had quite a few cases of this overtime and I think it would be nice to not have to add them 1 by 1.

flags = ["--serialize-diagnostics", "%{serialized_diagnostics_file}"],
this is another case

@lilygorsheneva

Copy link
Copy Markdown
Collaborator

IMO we should support someway for toolchains to add files like this in a generic way. we have had quite a few cases of this overtime and I think it would be nice to not have to add them 1 by 1.

flags = ["--serialize-diagnostics", "%{serialized_diagnostics_file}"],

this is another case

Could we have the toolchain provide semi-arbitrary (we'd need to define what context objects we can pass) starlark functions to the rules?

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.

Support Clang 9's "ftime-trace"

6 participants