Compose C++ toolchain variables in Starlark

cc/private/compile/compile.bzl

@@ -562,10 +562,7 @@ def _create_scan_deps_action(
             override_extension = extensions.CC_SOURCE[0],
         ),
     )
-    compile_variables = _cc_internal.combine_cc_toolchain_variables(
-        common_toolchain_variables,
-        specific_compile_build_variables,
-    )
+    compile_variables = common_toolchain_variables | specific_compile_build_variables
     _create_compile_action(
         action_construction_context = action_construction_context,
         cc_compilation_context = cc_compilation_context,
@@ -1371,10 +1368,7 @@ def _create_cc_compile_actions(
                 label = source_label,
             ),
         )
-        compile_variables = _cc_internal.combine_cc_toolchain_variables(
-            common_compile_build_variables,
-            specific_compile_build_variables,
-        )
+        compile_variables = common_compile_build_variables | specific_compile_build_variables
 
         # This creates the action to parse a header file.
         # If we generate pic actions, we prefer the header actions to use the pic artifacts.
@@ -1709,10 +1703,7 @@ def _create_compile_source_action(
         use_pic = use_pic,
         lto_indexing_file = lto_indexing_file,
         action_name = action_name,
-        compile_build_variables = _cc_internal.combine_cc_toolchain_variables(
-            common_compile_variables,
-            compile_variables,
-        ),
+        compile_build_variables = common_compile_variables | compile_variables,
         needs_include_validation = _starlark_cc_semantics.needs_include_validation(language),
         toolchain_type = _starlark_cc_semantics.toolchain,
         progress_message_prefix = progress_message_prefix,
@@ -1891,10 +1882,7 @@ def _create_temps_action(
         dotd_file = preprocess_dotd_file,
         diagnostics_file = preprocess_diagnostics_file,
         use_pic = use_pic,
-        compile_build_variables = _cc_internal.combine_cc_toolchain_variables(
-            common_compile_variables,
-            preprocess_compile_variables,
-        ),
+        compile_build_variables = common_compile_variables | preprocess_compile_variables,
         action_name = action_name,
         needs_include_validation = _starlark_cc_semantics.needs_include_validation(language),
         toolchain_type = _starlark_cc_semantics.toolchain,
@@ -1913,10 +1901,7 @@ def _create_temps_action(
         dotd_file = assembly_dotd_file,
         diagnostics_file = assembly_diagnostics_file,
         use_pic = use_pic,
-        compile_build_variables = _cc_internal.combine_cc_toolchain_variables(
-            common_compile_variables,
-            assembly_compile_variables,
-        ),
+        compile_build_variables = common_compile_variables | assembly_compile_variables,
         action_name = action_name,
         needs_include_validation = _starlark_cc_semantics.needs_include_validation(language),
         toolchain_type = _starlark_cc_semantics.toolchain,
@@ -2043,10 +2028,7 @@ def _create_module_codegen_action(
         fdo_build_variables = fdo_build_variables,
         additional_build_variables = {},
     )
-    compile_variables = _cc_internal.combine_cc_toolchain_variables(
-        common_toolchain_variables,
-        specific_compile_build_variables,
-    )
+    compile_variables = common_toolchain_variables | specific_compile_build_variables
 
     additional_inputs = []
 
@@ -2344,7 +2326,7 @@ def _create_compile_action(
         additional_include_scanning_roots = additional_include_scanning_roots,
         cc_compilation_context = cc_compilation_context,
         cc_toolchain = cc_toolchain,
-        compile_build_variables = compile_build_variables,
+        compile_build_variables = _cc_internal.cc_toolchain_variables(vars = compile_build_variables),
         configuration = configuration,
         copts_filter = copts_filter,
         diagnostics_file = diagnostics_file,

cc/private/compile/compile_action_templates.bzl

@@ -201,9 +201,8 @@ def _create_compile_action_template(
         cc_toolchain = cc_toolchain,
         configuration = configuration,
         feature_configuration = feature_configuration,
-        compile_build_variables = _cc_internal.combine_cc_toolchain_variables(
-            common_compile_build_variables,
-            specific_compile_build_variables,
+        compile_build_variables = _cc_internal.cc_toolchain_variables(
+            vars = common_compile_build_variables | specific_compile_build_variables,
         ),
         source = source_dir,
         additional_compilation_inputs = additional_compilation_inputs,

cc/private/compile/compile_build_variables.bzl

@@ -183,7 +183,12 @@ def create_compile_variables(
         additional_build_variables = additional_build_variables,
         user_compile_flags = user_compile_flags or [],
     )
-    return _cc_internal.combine_cc_toolchain_variables(cc_toolchain._build_variables, common_vars, variables)
+    duplicate_variables = sorted([key for key in variables if key in common_vars])
+    if duplicate_variables:
+        fail("Cannot overwrite existing variables: [" + ", ".join(duplicate_variables) + "]")
+    return _cc_internal.cc_toolchain_variables(
+        vars = cc_toolchain._build_variables_dict | common_vars | variables,
+    )
 
 # buildifier: disable=function-docstring
 def setup_common_compile_build_variables(
@@ -208,7 +213,7 @@ def setup_common_compile_build_variables(
         local_defines = cc_compilation_context.local_defines,
         external_include_dirs = cc_compilation_context.external_includes,
     )
-    return _cc_internal.combine_cc_toolchain_variables(cc_toolchain._build_variables, common_vars)
+    return cc_toolchain._build_variables_dict | common_vars
 
 def _setup_common_compile_build_variables_internal(
         *,
@@ -266,7 +271,7 @@ def _setup_common_compile_build_variables_internal(
 
     if external_include_dirs:
         result[_VARS.EXTERNAL_INCLUDE_PATHS] = external_include_dirs
-    return _cc_internal.cc_toolchain_variables(vars = result)
+    return result
 
 # Note: this method is side-effect free, callers should add fdo inputs to
 # cc_compile_action_builder themselves
@@ -290,7 +295,7 @@ def get_specific_compile_build_variables(
         user_compile_flags = [],
         additional_build_variables = {},
         fdo_build_variables = {}):
-    """Creates a CcToolchainVariables instance
+    """Creates a dictionary of compile build variables.
 
     Args:
         feature_configuration: (FeatureConfiguration)
@@ -314,7 +319,7 @@ def get_specific_compile_build_variables(
         fdo_build_variables: (dict{str,str})
 
     Returns:
-        (Variables)
+        (dict{str, object})
     """
     result = {}
 
@@ -360,7 +365,7 @@ def get_specific_compile_build_variables(
         result[_VARS.PIC] = ""
     result = result | additional_build_variables
     result = result | fdo_build_variables
-    return _cc_internal.cc_toolchain_variables(vars = result)
+    return result
 
 _SOURCE_TYPES_FOR_CXXOPTS = set(
     extensions.CC_SOURCE +

cc/private/link/lto_backends.bzl

@@ -268,7 +268,7 @@ def setup_common_lto_variables(
       feature_configuration: (feature_configuration) The feature configuration.
 
     Returns:
-      A CcToolchainVariables provider and a list[File] of additional inputs.
+      A dictionary of build variables and a list[File] of additional inputs.
     """
 
     build_variables = {}
@@ -285,12 +285,7 @@ def setup_common_lto_variables(
     if feature_configuration.is_enabled("cs_fdo_instrument"):
         build_variables["cs_fdo_instrument_path"] = cc_toolchain._cpp_configuration.cs_fdo_instrument()
 
-    build_variables = _cc_internal.combine_cc_toolchain_variables(
-        cc_toolchain._build_variables,
-        _cc_internal.cc_toolchain_variables(vars = build_variables),
-    )
-
-    return build_variables, additional_inputs
+    return cc_toolchain._build_variables_dict | build_variables, additional_inputs
 
 def create_lto_backend_artifacts(
         *,
@@ -327,7 +322,7 @@ def create_lto_backend_artifacts(
       cc_toolchain: (CcToolchainInfo) The C++ toolchain.
       use_pic: (bool) Whether to use PIC.
       should_create_per_object_debug_info: (bool) Whether to create per-object debug info.
-      build_variables: (CcToolchainVariables) Toolchain variables to use for argument expansion.
+      build_variables: (dict) Toolchain variables to use for argument expansion.
       additional_inputs: list[File] Additional file inputs required for generated actions.
       argv: (list[str]) The command line arguments to pass to the LTO backend.
 
@@ -340,17 +335,15 @@ def create_lto_backend_artifacts(
 
     create_shared_non_lto = all_bitcode_files == None
 
-    build_variables = _cc_internal.combine_cc_toolchain_variables(
-        build_variables,
-        _cc_internal.cc_toolchain_variables(vars = {
-            "user_compile_flags": _cc_internal.intern_string_sequence_variable_value(argv),
-        }),
-    )
+    build_variables = build_variables | {
+        "user_compile_flags": _cc_internal.intern_string_sequence_variable_value(argv),
+    }
+    native_build_variables = _cc_internal.cc_toolchain_variables(vars = build_variables)
 
     env = _cc_common_internal.get_environment_variables(
         feature_configuration = feature_configuration,
         action_name = "lto-backend",
-        variables = build_variables,
+        variables = native_build_variables,
     )
 
     obj = lto_obj_root_prefix + "/" + bitcode_file.path
@@ -378,7 +371,7 @@ def create_lto_backend_artifacts(
             feature_configuration = feature_configuration,
             additional_inputs = additional_inputs,
             env = env,
-            build_variables = build_variables,
+            build_variables = native_build_variables,
             use_pic = use_pic,
             all_bitcode_files = all_bitcode_files,
             index = index,
@@ -481,13 +474,12 @@ def _create_lto_backend_action(
         dwo_file,
         bitcode_file_path if bitcode_file_path != None else bitcode_artifact,
     )
-    _path_variables = _cc_internal.cc_toolchain_variables(vars = _path_variables)
-    build_variables = _cc_internal.combine_cc_toolchain_variables(build_variables, _path_variables)
-
     _cc_internal.create_lto_backend_action(
         actions = actions,
         feature_configuration = feature_configuration,
-        build_variables = build_variables,
+        build_variables = _cc_internal.cc_toolchain_variables(
+            vars = build_variables | _path_variables,
+        ),
         use_pic = use_pic,
         inputs = inputs,
         all_bitcode_files = bitcode_files,

tests/cc/common/compile_build_variables_tests.bzl

@@ -1,12 +1,48 @@
 """Tests for compile build variables."""
 
+load("@bazel_features//:features.bzl", "bazel_features")
 load("@rules_testing//lib:analysis_test.bzl", "test_suite")
 load("@rules_testing//lib:truth.bzl", "matching", "subjects")
 load("@rules_testing//lib:util.bzl", "TestingAspectInfo", "util")
+load("//cc:action_names.bzl", "ACTION_NAMES")
 load("//cc:cc_binary.bzl", _actual_cc_binary = "cc_binary")
+load("//cc:find_cc_toolchain.bzl", "CC_TOOLCHAIN_ATTRS", "find_cpp_toolchain", "use_cc_toolchain")
+load("//cc/common:cc_common.bzl", "cc_common")
 load("//tests/cc/testutil:cc_analysis_test.bzl", "cc_analysis_test")
 load("//tests/cc/testutil:link_action_subject.bzl", "link_action_subject")
 
+_CompileVariablesInfo = provider(
+    doc = "Arguments expanded from cc_common.create_compile_variables.",
+    fields = ["arguments"],
+)
+
+def _compile_variables_test_rule_impl(ctx):
+    cc_toolchain = find_cpp_toolchain(ctx)
+    feature_configuration = cc_common.configure_features(
+        ctx = ctx,
+        cc_toolchain = cc_toolchain,
+        requested_features = ["debug_variables"],
+    )
+    variables = cc_common.create_compile_variables(
+        cc_toolchain = cc_toolchain,
+        feature_configuration = feature_configuration,
+        source_file = "test.cc",
+        output_file = "test.o",
+        variables_extension = ctx.attr.variables_extension,
+    )
+    return [_CompileVariablesInfo(arguments = cc_common.get_memory_inefficient_command_line(
+        feature_configuration = feature_configuration,
+        action_name = ACTION_NAMES.cpp_compile,
+        variables = variables,
+    ))]
+
+_compile_variables_test_rule = rule(
+    implementation = _compile_variables_test_rule_impl,
+    attrs = CC_TOOLCHAIN_ATTRS | {"variables_extension": attr.string_dict()},
+    fragments = ["cpp"],
+    toolchains = use_cc_toolchain(),
+)
+
 # Wrap cc_binary to mock out common dependencies.
 def cc_binary(name, **kwargs):
     if "malloc" not in kwargs:
@@ -232,6 +268,51 @@ def _test_presence_of_sysroot_build_variable_impl(env, target):
     compile_action = _compile_action(env, target, "bin")
     _variable(compile_action, "sysroot").equals("/usr/grte/v1")
 
+def _test_compile_variables_extension_overrides_toolchain_variable(name, **kwargs):
+    util.helper_target(
+        _compile_variables_test_rule,
+        name = name + "/variables",
+        variables_extension = {"sysroot": "/overridden/sysroot"},
+    )
+    cc_analysis_test(
+        name = name,
+        impl = _test_compile_variables_extension_overrides_toolchain_variable_impl,
+        target = name + "/variables",
+        **kwargs
+    )
+
+def _test_compile_variables_extension_overrides_toolchain_variable_impl(env, target):
+    env.expect.that_collection(target[_CompileVariablesInfo].arguments).contains(
+        "--debug-var:sysroot=/overridden/sysroot",
+    )
+
+def _test_compile_variables_extension_rejects_duplicate_variables(name, **kwargs):
+    util.helper_target(
+        _compile_variables_test_rule,
+        name = name + "/variables",
+        variables_extension = {
+            "user_compile_flags": "duplicate flags",
+            "output_file": "duplicate output",
+            "source_file": "duplicate source",
+        },
+    )
+    cc_analysis_test(
+        name = name,
+        impl = _test_compile_variables_extension_rejects_duplicate_variables_impl,
+        target = name + "/variables",
+        expect_failure = True,
+        **kwargs
+    )
+
+def _test_compile_variables_extension_rejects_duplicate_variables_impl(env, target):
+    expected_error = "Cannot overwrite existing variables: [output_file, source_file, user_compile_flags]"
+    env.expect.that_target(target).failures().contains_predicate(
+        matching.custom(
+            "contains '{}'".format(expected_error),
+            lambda actual: expected_error in actual,
+        ),
+    )
+
 def _test_target_sysroot_without_platforms(name, **kwargs):
     util.helper_target(
         cc_binary,
@@ -390,21 +471,27 @@ def compile_build_variables_tests(name):
     Args:
         name: The name of the test suite.
     """
+    tests = [
+        _test_presence_of_basic_variables,
+        _test_presence_of_configuration_compile_flags,
+        _test_presence_of_conly_flags,
+        _test_cxx_flags_order,
+        _test_per_file_copts_are_in_user_compile_flags,
+        _test_host_per_file_copts_are_in_user_compile_flags,
+        _test_presence_of_sysroot_build_variable,
+        _test_target_sysroot_without_platforms,
+        _test_target_sysroot_with_platforms,
+        _test_presence_of_is_using_fission_variable,
+        _test_presence_of_is_using_fission_and_per_debug_object_file_variables_with_thinlto,
+        _test_presence_of_per_object_debug_file_build_variable,
+        _test_presence_of_min_os_version_build_variable,
+    ]
+    if bazel_features.cc.cc_common_is_in_rules_cc:
+        tests.extend([
+            _test_compile_variables_extension_overrides_toolchain_variable,
+            _test_compile_variables_extension_rejects_duplicate_variables,
+        ])
     test_suite(
         name = name,
-        tests = [
-            _test_presence_of_basic_variables,
-            _test_presence_of_configuration_compile_flags,
-            _test_presence_of_conly_flags,
-            _test_cxx_flags_order,
-            _test_per_file_copts_are_in_user_compile_flags,
-            _test_host_per_file_copts_are_in_user_compile_flags,
-            _test_presence_of_sysroot_build_variable,
-            _test_target_sysroot_without_platforms,
-            _test_target_sysroot_with_platforms,
-            _test_presence_of_is_using_fission_variable,
-            _test_presence_of_is_using_fission_and_per_debug_object_file_variables_with_thinlto,
-            _test_presence_of_per_object_debug_file_build_variable,
-            _test_presence_of_min_os_version_build_variable,
-        ],
+        tests = tests,
     )