From 184ffe54114441d9c59b9c03b2c539b859396c8e Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Wed, 3 Sep 2025 14:21:06 -0700 Subject: [PATCH 1/3] Move magic compiler wrapper args to env vars This lets us avoid trying to parse these values out of various arguments and instead we just get them through the environment. --- crosstool/cc_toolchain_config.bzl | 24 +++++++------ crosstool/universal_exec_tool.bzl | 1 + crosstool/wrapped_clang.cc | 45 ++++++------------------- test/linking_tests.bzl | 14 +++++--- test/rules/action_command_line_test.bzl | 36 ++++++++++++++++++++ 5 files changed, 70 insertions(+), 50 deletions(-) diff --git a/crosstool/cc_toolchain_config.bzl b/crosstool/cc_toolchain_config.bzl index 8002332f..f381c16e 100644 --- a/crosstool/cc_toolchain_config.bzl +++ b/crosstool/cc_toolchain_config.bzl @@ -759,12 +759,13 @@ please file an issue at https://github.com/bazelbuild/apple_support/issues/new strip_debug_symbols_feature = feature( name = "strip_debug_symbols", - flag_sets = [ - flag_set( + env_sets = [ + env_set( actions = _DYNAMIC_LINK_ACTIONS, - flag_groups = [ - flag_group( - flags = ["STRIP_DEBUG_SYMBOLS"], + env_entries = [ + env_entry( + key = "STRIP_DEBUG_SYMBOLS", + value = "true", expand_if_available = "strip_debug_symbols", ), ], @@ -1815,13 +1816,14 @@ please file an issue at https://github.com/bazelbuild/apple_support/issues/new ], flag_groups = [flag_group(flags = ["-g"])], ), - flag_set( + ], + env_sets = [ + env_set( actions = _DYNAMIC_LINK_ACTIONS, - flag_groups = [ - flag_group( - flags = [ - "DSYM_HINT_DSYM_PATH=%{dsym_path}", - ], + env_entries = [ + env_entry( + key = "DSYM_HINT_DSYM_PATH", + value = "%{dsym_path}", # We need to check this for backwards compatibility with bazel 7 expand_if_available = "dsym_path", ), diff --git a/crosstool/universal_exec_tool.bzl b/crosstool/universal_exec_tool.bzl index 898d2410..f7ab0adc 100644 --- a/crosstool/universal_exec_tool.bzl +++ b/crosstool/universal_exec_tool.bzl @@ -39,6 +39,7 @@ env -i \ -arch arm64 \ -arch x86_64 \ -O3 \ + -g \ -o $@ \ $(SRCS) """, diff --git a/crosstool/wrapped_clang.cc b/crosstool/wrapped_clang.cc index a5076b30..3d7ecc19 100644 --- a/crosstool/wrapped_clang.cc +++ b/crosstool/wrapped_clang.cc @@ -12,16 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. // -// wrapped_clang.cc: Pass args to 'xcrun clang' and zip dsym files. +// wrapped_clang.cc: Pass args to 'xcrun clang' and optionally produce dSYM +// files. // -// wrapped_clang passes its args to clang, but also supports a separate set of -// invocations to generate dSYM files. If "DSYM_HINT" flags are passed in, they -// are used to construct that separate set of invocations (instead of being -// passed to clang). -// The following "DSYM_HINT" flags control dsym generation. If any one if these -// are passed in, then they all must be passed in. -// "LINKED_BINARY": Workspace-relative path to binary output of the link action. -// "DSYM_HINT_DSYM_PATH": Workspace-relative path to dSYM dwarf file. #include #include @@ -256,14 +249,12 @@ static std::unique_ptr WriteResponseFile( void ProcessArgument(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string &linked_binary, std::string &dsym_path, - bool &strip_debug_symbols, std::string toolchain_path, + std::string toolchain_path, std::function consumer); bool ProcessResponseFile(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string &linked_binary, std::string &dsym_path, - bool &strip_debug_symbols, std::string toolchain_path, + std::string toolchain_path, std::function consumer) { auto path = arg.substr(1); std::ifstream original_file(path); @@ -277,7 +268,6 @@ bool ProcessResponseFile(const std::string arg, const std::string developer_dir, // Arguments in response files might be quoted/escaped, so we need to // unescape them ourselves. ProcessArgument(Unescape(arg_from_file), developer_dir, sdk_root, cwd, - linked_binary, dsym_path, strip_debug_symbols, toolchain_path, consumer); } @@ -360,29 +350,17 @@ std::string GetToolchainPath(const std::string &toolchain_id) { void ProcessArgument(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string &linked_binary, std::string &dsym_path, - bool &strip_debug_symbols, std::string toolchain_path, + std::string toolchain_path, std::function consumer) { auto new_arg = arg; if (arg[0] == '@') { - if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd, linked_binary, - dsym_path, strip_debug_symbols, toolchain_path, + if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd, + toolchain_path, consumer)) { return; } } - if (SetArgIfFlagPresent(arg, "LINKED_BINARY", &linked_binary)) { - return; - } - if (SetArgIfFlagPresent(arg, "DSYM_HINT_DSYM_PATH", &dsym_path)) { - return; - } - if (arg == "STRIP_DEBUG_SYMBOLS") { - strip_debug_symbols = true; - return; - } - FindAndReplace("__BAZEL_EXECUTION_ROOT__", cwd, &new_arg); FindAndReplace("__BAZEL_EXECUTION_ROOT_CANONICAL__", GetCanonicalPath(cwd), &new_arg); FindAndReplace("__BAZEL_XCODE_DEVELOPER_DIR__", developer_dir, &new_arg); @@ -436,8 +414,6 @@ int main(int argc, char *argv[]) { std::string developer_dir = GetMandatoryEnvVar("DEVELOPER_DIR"); std::string sdk_root = GetMandatoryEnvVar("SDKROOT"); - std::string linked_binary, dsym_path; - bool strip_debug_symbols = false; const std::string cwd = GetCurrentDirectory(); std::vector invocation_args = {"/usr/bin/xcrun", tool_name}; @@ -449,8 +425,8 @@ int main(int argc, char *argv[]) { for (int i = 1; i < argc; i++) { std::string arg(argv[i]); - ProcessArgument(arg, developer_dir, sdk_root, cwd, linked_binary, dsym_path, - strip_debug_symbols, toolchain_path, consumer); + ProcessArgument(arg, developer_dir, sdk_root, cwd, + toolchain_path, consumer); } char *modulemap = getenv("APPLE_SUPPORT_MODULEMAP"); @@ -515,7 +491,8 @@ int main(int argc, char *argv[]) { // When stripping is requested, we should still strip the binary // before returning - if (strip_debug_symbols) { + const char *strip_debug_symbols = getenv("STRIP_DEBUG_SYMBOLS"); + if (strip_debug_symbols != nullptr) { std::vector strip_args = {"/usr/bin/xcrun", "strip", "-S", linked_binary}; if (!RunSubProcess(strip_args)) { diff --git a/test/linking_tests.bzl b/test/linking_tests.bzl index 5440b9d6..8c11d273 100644 --- a/test/linking_tests.bzl +++ b/test/linking_tests.bzl @@ -65,9 +65,11 @@ def linking_test_suite(name): ], not_expected_argv = [ "-g", - "DSYM_HINT_DSYM_PATH", "-dead_strip", ], + not_expected_env_keys = [ + "DSYM_HINT_DSYM_PATH", + ], mnemonic = "ObjcLink", target_under_test = "//test/test_data:macos_binary", ) @@ -88,9 +90,11 @@ def linking_test_suite(name): ], not_expected_argv = [ "-g", - "DSYM_HINT_DSYM_PATH", "-dead_strip", ], + not_expected_env_keys = [ + "DSYM_HINT_DSYM_PATH", + ], mnemonic = "ObjcLink", target_under_test = "//test/test_data:ios_binary", ) @@ -184,7 +188,8 @@ def linking_test_suite(name): tags = [name], expected_argv = [ "-g", - "LINKED_BINARY", + ], + expected_env_keys = [ "DSYM_HINT_DSYM_PATH", ], mnemonic = "ObjcLink", @@ -194,8 +199,7 @@ def linking_test_suite(name): dsym_test( name = "{}_generate_cpp_dsym_test".format(name), tags = [name], - expected_argv = [ - "LINKED_BINARY", + expected_env_keys = [ "DSYM_HINT_DSYM_PATH", ], mnemonic = "CppLink", diff --git a/test/rules/action_command_line_test.bzl b/test/rules/action_command_line_test.bzl index 83852cd8..3257003e 100644 --- a/test/rules/action_command_line_test.bzl +++ b/test/rules/action_command_line_test.bzl @@ -102,6 +102,28 @@ def _action_command_line_test_impl(ctx): ), ) + for expected_key in ctx.attr.expected_env_keys: + if expected_key not in action.env: + unittest.fail( + env, + "{}expected env to contain key '{}', but it did not: {}".format( + message_prefix, + expected_key, + action.env, + ), + ) + + for not_expected_key in ctx.attr.not_expected_env_keys: + if not_expected_key in action.env: + unittest.fail( + env, + "{}expected env to not contain key '{}', but it did: {}".format( + message_prefix, + not_expected_key, + action.env, + ), + ) + return analysistest.end(env) def make_action_command_line_test_rule(config_settings = {}): @@ -132,6 +154,20 @@ space-delimited string. A list of strings representing substrings expected not to appear in the action command line, after concatenating all command line arguments into a single space-delimited string. +""", + ), + "expected_env_keys": attr.string_list( + mandatory = False, + doc = """\ +A list of strings representing environment variable keys expected to be set +in the action environment. +""", + ), + "not_expected_env_keys": attr.string_list( + mandatory = False, + doc = """\ +A list of strings representing environment variable keys expected not to be +set in the action environment. """, ), "mnemonic": attr.string( From 0136584573047f15e41370a7ef6be3a6b2a90cd0 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Wed, 3 Sep 2025 14:25:36 -0700 Subject: [PATCH 2/3] Discard changes to crosstool/universal_exec_tool.bzl --- crosstool/universal_exec_tool.bzl | 1 - 1 file changed, 1 deletion(-) diff --git a/crosstool/universal_exec_tool.bzl b/crosstool/universal_exec_tool.bzl index f7ab0adc..898d2410 100644 --- a/crosstool/universal_exec_tool.bzl +++ b/crosstool/universal_exec_tool.bzl @@ -39,7 +39,6 @@ env -i \ -arch arm64 \ -arch x86_64 \ -O3 \ - -g \ -o $@ \ $(SRCS) """, From 3e8701bc4e69715c8a997fa1c00ce6fbc19748a6 Mon Sep 17 00:00:00 2001 From: Keith Smiley Date: Fri, 19 Dec 2025 15:27:25 -0800 Subject: [PATCH 3/3] fix conflicts --- crosstool/wrapped_clang.cc | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/crosstool/wrapped_clang.cc b/crosstool/wrapped_clang.cc index 3d7ecc19..d08afbb3 100644 --- a/crosstool/wrapped_clang.cc +++ b/crosstool/wrapped_clang.cc @@ -249,12 +249,12 @@ static std::unique_ptr WriteResponseFile( void ProcessArgument(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string toolchain_path, + std::string &linked_binary, std::string toolchain_path, std::function consumer); bool ProcessResponseFile(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string toolchain_path, + std::string &linked_binary, std::string toolchain_path, std::function consumer) { auto path = arg.substr(1); std::ifstream original_file(path); @@ -268,7 +268,7 @@ bool ProcessResponseFile(const std::string arg, const std::string developer_dir, // Arguments in response files might be quoted/escaped, so we need to // unescape them ourselves. ProcessArgument(Unescape(arg_from_file), developer_dir, sdk_root, cwd, - toolchain_path, consumer); + linked_binary, toolchain_path, consumer); } return true; @@ -350,17 +350,20 @@ std::string GetToolchainPath(const std::string &toolchain_id) { void ProcessArgument(const std::string arg, const std::string developer_dir, const std::string sdk_root, const std::string cwd, - std::string toolchain_path, + std::string &linked_binary, std::string toolchain_path, std::function consumer) { auto new_arg = arg; if (arg[0] == '@') { - if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd, - toolchain_path, - consumer)) { + if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd, linked_binary, + toolchain_path, consumer)) { return; } } + if (SetArgIfFlagPresent(arg, "LINKED_BINARY", &linked_binary)) { + return; + } + FindAndReplace("__BAZEL_EXECUTION_ROOT__", cwd, &new_arg); FindAndReplace("__BAZEL_EXECUTION_ROOT_CANONICAL__", GetCanonicalPath(cwd), &new_arg); FindAndReplace("__BAZEL_XCODE_DEVELOPER_DIR__", developer_dir, &new_arg); @@ -414,6 +417,7 @@ int main(int argc, char *argv[]) { std::string developer_dir = GetMandatoryEnvVar("DEVELOPER_DIR"); std::string sdk_root = GetMandatoryEnvVar("SDKROOT"); + std::string linked_binary; const std::string cwd = GetCurrentDirectory(); std::vector invocation_args = {"/usr/bin/xcrun", tool_name}; @@ -425,7 +429,7 @@ int main(int argc, char *argv[]) { for (int i = 1; i < argc; i++) { std::string arg(argv[i]); - ProcessArgument(arg, developer_dir, sdk_root, cwd, + ProcessArgument(arg, developer_dir, sdk_root, cwd, linked_binary, toolchain_path, consumer); } @@ -458,7 +462,9 @@ int main(int argc, char *argv[]) { output.close(); } - bool generate_dsym = !dsym_path.empty(); + const char *dsym_path_raw_value = getenv("DSYM_HINT_DSYM_PATH"); + bool generate_dsym = dsym_path_raw_value != nullptr; + bool strip_debug_symbols = getenv("STRIP_DEBUG_SYMBOLS") != nullptr; if (!generate_dsym && !strip_debug_symbols) { return 0; } @@ -471,6 +477,7 @@ int main(int argc, char *argv[]) { } if (generate_dsym) { + std::string dsym_path = dsym_path_raw_value; const std::string bundle_suffix = ".dSYM"; bool is_bundle = dsym_path.rfind(bundle_suffix) == dsym_path.length() - bundle_suffix.length(); @@ -491,8 +498,7 @@ int main(int argc, char *argv[]) { // When stripping is requested, we should still strip the binary // before returning - const char *strip_debug_symbols = getenv("STRIP_DEBUG_SYMBOLS"); - if (strip_debug_symbols != nullptr) { + if (strip_debug_symbols) { std::vector strip_args = {"/usr/bin/xcrun", "strip", "-S", linked_binary}; if (!RunSubProcess(strip_args)) {