From 1bea35e22e18c56818f1c485ab04d2272736ea45 Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Wed, 3 Feb 2021 12:07:35 -0800 Subject: [PATCH] Move toolchain- and command-line-provided compiler flags into action configurators. This change removes of the `SwiftToolchainInfo.command_line_copts` field, no longer treating those command line flags as an odd special case. It also ensures that flags from the toolchain are passed in a unified manner regardless of which rule registers the action. For example, Bazel-legacy Objective-C flags determined by compilation mode, like `-fstack-protector`, affect the compatibility of Clang modules and need to be passed both when compiling an explicit module and when compiling the Swift code that consumes those modules. This required a bit more shuffling around of the way we need to handle differing outptus for WMO, since the `--swiftcopt` flags would no longer be able to be scanned directly after the toolchain configuration. PiperOrigin-RevId: 355451642 --- swift/internal/actions.bzl | 12 +- swift/internal/compiling.bzl | 190 ++++++++++++++++++----- swift/internal/feature_names.bzl | 10 ++ swift/internal/providers.bzl | 7 - swift/internal/swift_toolchain.bzl | 22 ++- swift/internal/xcode_swift_toolchain.bzl | 34 ++-- 6 files changed, 203 insertions(+), 72 deletions(-) diff --git a/swift/internal/actions.bzl b/swift/internal/actions.bzl index 931d22f51..a411e60fe 100644 --- a/swift/internal/actions.bzl +++ b/swift/internal/actions.bzl @@ -131,7 +131,17 @@ def _apply_action_configs( prerequisites, args, ) - if action_inputs: + + # If we create an action configurator from a lambda that calls + # `Args.add*`, the result will be the `Args` objects (rather + # than `None`) because those methods return the same `Args` + # object for chaining. We can guard against this (and possibly + # other errors) by checking that the value is a struct. If it + # is, then it's not `None` and it probably came from the + # provider used by `swift_toolchain_config.config_result`. If + # it's some other kind of struct, then we'll error out trying to + # access the fields. + if type(action_inputs) == "struct": inputs.extend(action_inputs.inputs) transitive_inputs.extend(action_inputs.transitive_inputs) diff --git a/swift/internal/compiling.bzl b/swift/internal/compiling.bzl index 1e9df61fa..3d45d33c9 100644 --- a/swift/internal/compiling.bzl +++ b/swift/internal/compiling.bzl @@ -57,6 +57,8 @@ load( "SWIFT_FEATURE_USE_C_MODULES", "SWIFT_FEATURE_USE_GLOBAL_MODULE_CACHE", "SWIFT_FEATURE_VFSOVERLAY", + "SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS", + "SWIFT_FEATURE__WMO_IN_SWIFTCOPTS", ) load(":features.bzl", "are_all_features_enabled", "is_feature_enabled") load(":module_maps.bzl", "write_module_map") @@ -81,12 +83,32 @@ _SWIFTMODULES_VFS_ROOT = "/__build_bazel_rules_swift/swiftmodules" # when an API to obtain this is available. _DEFAULT_WMO_THREAD_COUNT = 12 -def compile_action_configs(): +# Swift command line flags that enable whole module optimization. (This +# dictionary is used as a set for quick lookup; the values are irrelevant.) +_WMO_FLAGS = { + "-wmo": True, + "-whole-module-optimization": True, + "-force-single-frontend-invocation": True, +} + +def compile_action_configs( + *, + additional_objc_copts = [], + additional_swiftc_copts = []): """Returns the list of action configs needed to perform Swift compilation. Toolchains must add these to their own list of action configs so that compilation actions will be correctly configured. + Args: + additional_objc_copts: An optional list of additional Objective-C + compiler flags that should be passed (preceded by `-Xcc`) to Swift + compile actions *and* Swift explicit module precompile actions after + any other toolchain- or user-provided flags. + additional_swiftc_copts: An optional list of additional Swift compiler + flags that should be passed to Swift compile actions only after any + other toolchain- or user-provided flags. + Returns: The list of action configs needed to perform compilation. """ @@ -542,10 +564,14 @@ def compile_action_configs(): actions = [swift_action_names.COMPILE], configurators = [_batch_mode_configurator], features = [SWIFT_FEATURE_ENABLE_BATCH_MODE], - not_features = [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + not_features = [ + [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + [SWIFT_FEATURE__WMO_IN_SWIFTCOPTS], + ], ), - # Set the number of threads to use for WMO. + # Set the number of threads to use for WMO. (We can skip this if we know + # we'll already be applying `-num-threads` via `--swiftcopt` flags.) swift_toolchain_config.action_config( actions = [swift_action_names.COMPILE], configurators = [ @@ -556,7 +582,11 @@ def compile_action_configs(): False, ), ], - features = [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + features = [ + [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + [SWIFT_FEATURE__WMO_IN_SWIFTCOPTS], + ], + not_features = [SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS], ), swift_toolchain_config.action_config( actions = [swift_action_names.COMPILE], @@ -568,7 +598,10 @@ def compile_action_configs(): True, ), ], - not_features = [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + not_features = [ + [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + [SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS], + ], ), # Set the module name. @@ -601,16 +634,45 @@ def compile_action_configs(): ), ] - # NOTE: The position of this action config in the list is important, because - # it places user compile flags after flags added by the rules, allowing - # `copts` attributes and `--swiftcopt` flag values to override flags set by - # the rule implementations as a last resort. + # NOTE: The positions of these action configs in the list are important, + # because it places the `copts` attribute ("user compile flags") after flags + # added by the rules, and then the "additional objc" and "additional swift" + # flags follow those, which are `--objccopt` and `--swiftcopt` flags from + # the command line that should override even the flags specified in the + # `copts` attribute. action_configs.append( swift_toolchain_config.action_config( actions = [swift_action_names.COMPILE], configurators = [_user_compile_flags_configurator], ), ) + if additional_objc_copts: + action_configs.append( + swift_toolchain_config.action_config( + actions = [ + swift_action_names.COMPILE, + swift_action_names.PRECOMPILE_C_MODULE, + ], + configurators = [ + lambda _, args: args.add_all( + additional_objc_copts, + before_each = "-Xcc", + ), + ], + ), + ) + if additional_swiftc_copts: + action_configs.append( + swift_toolchain_config.action_config( + # TODO(allevato): Determine if there are any uses of + # `-Xcc`-prefixed flags that need to be added to explicit module + # actions, or if we should advise against/forbid that. + actions = [swift_action_names.COMPILE], + configurators = [ + lambda _, args: args.add_all(additional_swiftc_copts), + ], + ), + ) action_configs.append( swift_toolchain_config.action_config( @@ -1051,9 +1113,34 @@ def _is_wmo_manually_requested(user_compile_flags): Returns: True if WMO is enabled in the given list of flags. """ - return ("-wmo" in user_compile_flags or - "-whole-module-optimization" in user_compile_flags or - "-force-single-frontend-invocation" in user_compile_flags) + for copt in user_compile_flags: + if copt in _WMO_FLAGS: + return True + return False + +def features_from_swiftcopts(swiftcopts): + """Returns a list of features to enable based on `--swiftcopt` flags. + + Since `--swiftcopt` flags are hooked into the action configuration when the + toolchain is configured, it's not possible for individual actions to query + them easily if those flags may determine the nature of outputs (for example, + single- vs. multi-threaded WMO). The toolchain can call this function to map + those flags to private features that can be queried instead. + + Args: + swiftcopts: The list of command line flags that were passed using + `--swiftcopt`. + + Returns: + A list (possibly empty) of strings denoting feature names that should be + enabled on the toolchain. + """ + features = [] + if _is_wmo_manually_requested(user_compile_flags = swiftcopts): + features.append(SWIFT_FEATURE__WMO_IN_SWIFTCOPTS) + if _find_num_threads_flag_value(user_compile_flags = swiftcopts) == 1: + features.append(SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS) + return features def _index_while_building_configurator(prerequisites, args): """Adds flags for index-store generation to the command line.""" @@ -1222,7 +1309,7 @@ def compile( module_name = module_name, srcs = srcs, target_name = target_name, - user_compile_flags = copts + swift_toolchain.command_line_copts, + user_compile_flags = copts, ) all_compile_outputs = compact([ # The `.swiftmodule` file is explicitly listed as the first output @@ -1308,7 +1395,7 @@ def compile( source_files = srcs, transitive_modules = transitive_modules, transitive_swiftmodules = transitive_swiftmodules, - user_compile_flags = copts + swift_toolchain.command_line_copts, + user_compile_flags = copts, vfsoverlay_file = vfsoverlay_file, vfsoverlay_search_path = _SWIFTMODULES_VFS_ROOT, # Merge the compile outputs into the prerequisites. @@ -1592,12 +1679,8 @@ def _declare_compile_outputs( # Now, declare outputs like object files for which there may be one or many, # depending on the compilation mode. - is_wmo_implied_by_features = are_all_features_enabled( - feature_configuration = feature_configuration, - feature_names = [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], - ) output_nature = _emitted_output_nature( - is_wmo_implied_by_features = is_wmo_implied_by_features, + feature_configuration = feature_configuration, user_compile_flags = user_compile_flags, ) @@ -2105,7 +2188,32 @@ def _disable_autolink_framework_copts(framework_name): ], ) -def _emitted_output_nature(is_wmo_implied_by_features, user_compile_flags): +def _find_num_threads_flag_value(user_compile_flags): + """Finds the value of the `-num-threads` flag. + + This function looks for both forms of the flag (`-num-threads X` and + `-num-threads=X`) and returns the corresponding value if found. If the flag + is present multiple times, the last value is the one returned. + + Args: + user_compile_flags: The options passed into the compile action. + + Returns: + The numeric value of the `-num-threads` flag if found, otherwise `None`. + """ + num_threads = None + saw_space_separated_num_threads = False + for copt in user_compile_flags: + if saw_space_separated_num_threads: + saw_space_separated_num_threads = False + num_threads = _safe_int(copt) + elif copt == "-num-threads": + saw_space_separated_num_threads = True + elif copt.startswith("-num-threads="): + num_threads = _safe_int(copt.split("=")[1]) + return num_threads + +def _emitted_output_nature(feature_configuration, user_compile_flags): """Returns information about the nature of emitted compilation outputs. The compiler emits a single object if it is invoked with whole-module @@ -2115,8 +2223,8 @@ def _emitted_output_nature(is_wmo_implied_by_features, user_compile_flags): thread count,_ so we have to treat that case separately. Args: - is_wmo_implied_by_features: Whether WMO is implied by features set in - the feature configuration. + feature_configuration: The feature configuration for the current + compilation. user_compile_flags: The options passed into the compile action. Returns: @@ -2130,32 +2238,28 @@ def _emitted_output_nature(is_wmo_implied_by_features, user_compile_flags): compilation action with the given flags. """ is_wmo = ( - is_wmo_implied_by_features or + is_feature_enabled( + feature_configuration = feature_configuration, + feature_name = SWIFT_FEATURE__WMO_IN_SWIFTCOPTS, + ) or + are_all_features_enabled( + feature_configuration = feature_configuration, + feature_names = [SWIFT_FEATURE_OPT, SWIFT_FEATURE_OPT_USES_WMO], + ) or _is_wmo_manually_requested(user_compile_flags) ) - saw_space_separated_num_threads = False - - # If WMO is enabled, the action config will automatically add - # `-num-threads 12` to the command line. We need to stage that as our - # initial default here to ensure that we return the right value if the user - # compile flags don't otherwise override it. - num_threads = _DEFAULT_WMO_THREAD_COUNT if is_wmo else 1 - - for copt in user_compile_flags: - if saw_space_separated_num_threads: - saw_space_separated_num_threads = False - num_threads = _safe_int(copt) - elif copt == "-num-threads": - saw_space_separated_num_threads = True - elif copt.startswith("-num-threads="): - num_threads = _safe_int(copt.split("=")[1]) - - if not num_threads: - fail("The value of '-num-threads' must be a positive integer.") + # We check the feature first because that implies that `-num-threads 1` was + # present in `--swiftcopt`, which overrides all other flags (like the user + # compile flags, which come from the target's `copts`). Only fallback to + # checking the flags if the feature is disabled. + is_single_threaded = is_feature_enabled( + feature_configuration = feature_configuration, + feature_name = SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS, + ) or _find_num_threads_flag_value(user_compile_flags) == 1 return struct( - emits_multiple_objects = not (is_wmo and num_threads == 1), + emits_multiple_objects = not (is_wmo and is_single_threaded), emits_partial_modules = not is_wmo, ) diff --git a/swift/internal/feature_names.bzl b/swift/internal/feature_names.bzl index 8b12b2e3d..3efd19a0b 100644 --- a/swift/internal/feature_names.bzl +++ b/swift/internal/feature_names.bzl @@ -216,3 +216,13 @@ SWIFT_FEATURE_NO_EMBED_DEBUG_MODULE = "swift.no_embed_debug_module" # files where the protoc command line might not be crafted correctly, so it # remains opt in. SWIFT_FEATURE_GENERATE_FROM_RAW_PROTO_FILES = "swift.generate_from_raw_proto_files" + +# A private feature that is set by the toolchain if a flag enabling WMO was +# passed on the command line using `--swiftcopt`. Users should never manually +# enable, disable, or query this feature. +SWIFT_FEATURE__WMO_IN_SWIFTCOPTS = "swift._wmo_in_swiftcopts" + +# A private feature that is set by the toolchain if the flags `-num-threads 1` +# were passed on the command line using `--swiftcopt`. Users should never +# manually enable, disable, or query this feature. +SWIFT_FEATURE__NUM_THREADS_1_IN_SWIFTCOPTS = "swift._num_threads_1_in_swiftcopts" diff --git a/swift/internal/providers.bzl b/swift/internal/providers.bzl index 4d377c776..6d6fbfd05 100644 --- a/swift/internal/providers.bzl +++ b/swift/internal/providers.bzl @@ -69,13 +69,6 @@ using this toolchain. "cc_toolchain_info": """\ The `cc_common.CcToolchainInfo` provider from the Bazel C++ toolchain that this Swift toolchain depends on. -""", - "command_line_copts": """\ -`List` of `strings`. Flags that were passed to Bazel using the `--swiftcopt` -command line flag. These flags have the highest precedence; they are added to -compilation command lines after the toolchain default flags -(`SwiftToolchainInfo.swiftc_copts`) and after flags specified in the `copts` -attributes of Swift targets. """, "cpu": """\ `String`. The CPU architecture that the toolchain is targeting. diff --git a/swift/internal/swift_toolchain.bzl b/swift/internal/swift_toolchain.bzl index 2a1173c45..8f1a24a69 100644 --- a/swift/internal/swift_toolchain.bzl +++ b/swift/internal/swift_toolchain.bzl @@ -25,7 +25,7 @@ load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain") load(":actions.bzl", "swift_action_names") load(":attrs.bzl", "swift_toolchain_driver_attrs") load(":autolinking.bzl", "autolink_extract_action_configs") -load(":compiling.bzl", "compile_action_configs") +load(":compiling.bzl", "compile_action_configs", "features_from_swiftcopts") load(":debugging.bzl", "modulewrap_action_configs") load( ":feature_names.bzl", @@ -86,14 +86,20 @@ def _all_tool_configs( ), } -def _all_action_configs(): +def _all_action_configs(additional_swiftc_copts): """Returns the action configurations for the Swift toolchain. + Args: + additional_swiftc_copts: Additional Swift compiler flags obtained from + the `swift` configuration fragment. + Returns: A list of action configurations for the toolchain. """ return ( - compile_action_configs() + + compile_action_configs( + additional_swift_copts = additional_swiftc_copts, + ) + modulewrap_action_configs() + autolink_extract_action_configs() ) @@ -170,7 +176,10 @@ def _swift_toolchain_impl(ctx): # Combine build mode features, autoconfigured features, and required # features. - requested_features = features_for_build_modes(ctx) + requested_features = ( + features_for_build_modes(ctx) + + features_from_swiftcopts(swiftcopts = ctx.fragments.swift.copts()) + ) requested_features.extend([ SWIFT_FEATURE_NO_GENERATED_HEADER, SWIFT_FEATURE_NO_GENERATED_MODULE_MAP, @@ -191,7 +200,9 @@ def _swift_toolchain_impl(ctx): use_param_file = SWIFT_FEATURE_USE_RESPONSE_FILES in ctx.features, additional_tools = [ctx.file.version_file], ) - all_action_configs = _all_action_configs() + all_action_configs = _all_action_configs( + additional_swiftc_copts = ctx.fragments.swift.copts(), + ) # TODO(allevato): Move some of the remaining hardcoded values, like object # format and Obj-C interop support, to attributes so that we can remove the @@ -201,7 +212,6 @@ def _swift_toolchain_impl(ctx): action_configs = all_action_configs, all_files = depset(all_files), cc_toolchain_info = cc_toolchain, - command_line_copts = ctx.fragments.swift.copts(), cpu = ctx.attr.arch, linker_opts_producer = linker_opts_producer, object_format = "elf", diff --git a/swift/internal/xcode_swift_toolchain.bzl b/swift/internal/xcode_swift_toolchain.bzl index 5533eb6b7..2266a5479 100644 --- a/swift/internal/xcode_swift_toolchain.bzl +++ b/swift/internal/xcode_swift_toolchain.bzl @@ -19,14 +19,13 @@ toolchain package. If you are looking for rules to build Swift code using this toolchain, see `swift.bzl`. """ -load("@bazel_skylib//lib:collections.bzl", "collections") load("@bazel_skylib//lib:dicts.bzl", "dicts") load("@bazel_skylib//lib:partial.bzl", "partial") load("@bazel_skylib//lib:paths.bzl", "paths") load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain") load(":actions.bzl", "swift_action_names") load(":attrs.bzl", "swift_toolchain_driver_attrs") -load(":compiling.bzl", "compile_action_configs") +load(":compiling.bzl", "compile_action_configs", "features_from_swiftcopts") load( ":feature_names.bzl", "SWIFT_FEATURE_BITCODE_EMBEDDED", @@ -105,11 +104,7 @@ def _command_line_objc_copts(compilation_mode, objc_fragment): ] clang_copts = objc_fragment.copts + legacy_copts - - return collections.before_each( - "-Xcc", - [copt for copt in clang_copts if copt != "-g"], - ) + return [copt for copt in clang_copts if copt != "-g"] def _platform_developer_framework_dir(apple_toolchain, apple_fragment): """Returns the Developer framework directory for the platform. @@ -319,6 +314,8 @@ def _resource_directory_configurator(developer_dir, prerequisites, args): ) def _all_action_configs( + additional_objc_copts, + additional_swiftc_copts, apple_fragment, apple_toolchain, needs_resource_directory, @@ -326,6 +323,11 @@ def _all_action_configs( """Returns the action configurations for the Swift toolchain. Args: + additional_objc_copts: Additional Objective-C compiler flags obtained + from the `objc` configuration fragment (and legacy flags that were + previously passed directly by Bazel). + additional_swiftc_copts: Additional Swift compiler flags obtained from + the `swift` configuration fragment. apple_fragment: The `apple` configuration fragment. apple_toolchain: The `apple_common.apple_toolchain()` object. needs_resource_directory: If True, the toolchain needs the resource @@ -441,7 +443,10 @@ def _all_action_configs( ), ) - action_configs.extend(compile_action_configs()) + action_configs.extend(compile_action_configs( + additional_objc_copts = additional_objc_copts, + additional_swiftc_copts = additional_swiftc_copts, + )) return action_configs def _all_tool_configs( @@ -654,7 +659,7 @@ def _xcode_swift_toolchain_impl(ctx): requested_features = features_for_build_modes( ctx, objc_fragment = ctx.fragments.objc, - ) + ) + features_from_swiftcopts(swiftcopts = ctx.fragments.swift.copts()) requested_features.extend(ctx.features) requested_features.append(SWIFT_FEATURE_BUNDLED_XCTESTS) requested_features.extend( @@ -682,11 +687,6 @@ def _xcode_swift_toolchain_impl(ctx): requested_features.append(SWIFT_FEATURE_SUPPORTS_LIBRARY_EVOLUTION) requested_features.append(SWIFT_FEATURE_SUPPORTS_PRIVATE_DEPS) - command_line_copts = _command_line_objc_copts( - ctx.var["COMPILATION_MODE"], - ctx.fragments.objc, - ) + ctx.fragments.swift.copts() - env = _xcode_env(platform = platform, xcode_config = xcode_config) execution_requirements = _xcode_execution_requirements( xcode_config = xcode_config, @@ -702,6 +702,11 @@ def _xcode_swift_toolchain_impl(ctx): xcode_config = xcode_config, ) all_action_configs = _all_action_configs( + additional_objc_copts = _command_line_objc_copts( + ctx.var["COMPILATION_MODE"], + ctx.fragments.objc, + ), + additional_swiftc_copts = ctx.fragments.swift.copts(), apple_fragment = apple_fragment, apple_toolchain = apple_toolchain, needs_resource_directory = swift_executable or toolchain_root, @@ -720,7 +725,6 @@ def _xcode_swift_toolchain_impl(ctx): action_configs = all_action_configs, all_files = depset(all_files), cc_toolchain_info = cc_toolchain, - command_line_copts = command_line_copts, cpu = cpu, linker_opts_producer = linker_opts_producer, object_format = "macho",