Move magic compiler wrapper args to env vars

keith · keith · commit 009148abed4c · 2025-09-03T14:21:37.000-07:00
This lets us avoid trying to parse these values out of various arguments
and instead we just get them through the environment.
diff --git a/crosstool/cc_toolchain_config.bzl b/crosstool/cc_toolchain_config.bzl
@@ -863,12 +863,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",
                     ),
                 ],
@@ -2093,14 +2094,20 @@ 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_LINKED_BINARY=%{output_execpath}",
-                            "DSYM_HINT_DSYM_PATH=%{dsym_path}",
-                        ],
+                env_entries = [
+                    env_entry(
+                        key = "DSYM_HINT_LINKED_BINARY",
+                        value = "%{output_execpath}",
+                        # We need to check this for backwards compatibility with bazel 7
+                        expand_if_available = "dsym_path",
+                    ),
+                    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
@@ -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
@@ -12,17 +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.
-// "DSYM_HINT_LINKED_BINARY": Workspace-relative path to binary output of the
-//    link action generating the dsym file.
-// "DSYM_HINT_DSYM_PATH": Workspace-relative path to dSYM dwarf file.
 
 #include <libgen.h>
 #include <spawn.h>
@@ -259,16 +251,12 @@ static std::unique_ptr<TempFile> WriteResponseFile(
 
 void ProcessArgument(const std::string arg, const std::string developer_dir,
                      const std::string sdk_root, const std::string cwd,
-                     bool relative_ast_path, std::string &linked_binary,
-                     std::string &dsym_path, bool &strip_debug_symbols,
-                     std::string toolchain_path,
+                     bool relative_ast_path, std::string toolchain_path,
                      std::function<void(const std::string &)> consumer);
 
 bool ProcessResponseFile(const std::string arg, const std::string developer_dir,
                          const std::string sdk_root, const std::string cwd,
-                         bool relative_ast_path, std::string &linked_binary,
-                         std::string &dsym_path, bool &strip_debug_symbols,
-                         std::string toolchain_path,
+                         bool relative_ast_path, std::string toolchain_path,
                          std::function<void(const std::string &)> consumer) {
   auto path = arg.substr(1);
   std::ifstream original_file(path);
@@ -282,8 +270,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,
-                    relative_ast_path, linked_binary, dsym_path,
-                    strip_debug_symbols, toolchain_path, consumer);
+                    relative_ast_path, toolchain_path, consumer);
   }
 
   return true;
@@ -339,30 +326,16 @@ 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,
-                     bool relative_ast_path, std::string &linked_binary,
-                     std::string &dsym_path, bool &strip_debug_symbols,
-                     std::string toolchain_path,
+                     bool relative_ast_path, std::string toolchain_path,
                      std::function<void(const std::string &)> consumer) {
   auto new_arg = arg;
   if (arg[0] == '@') {
     if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd,
-                            relative_ast_path, linked_binary, dsym_path,
-                            strip_debug_symbols, toolchain_path, consumer)) {
+                            relative_ast_path, toolchain_path, consumer)) {
       return;
     }
   }
 
-  if (SetArgIfFlagPresent(arg, "DSYM_HINT_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_XCODE_DEVELOPER_DIR__", developer_dir, &new_arg);
   FindAndReplace("__BAZEL_XCODE_SDKROOT__", sdk_root, &new_arg);
@@ -428,8 +401,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<std::string> invocation_args = {"/usr/bin/xcrun", tool_name};
@@ -443,7 +414,6 @@ int main(int argc, char *argv[]) {
     std::string arg(argv[i]);
 
     ProcessArgument(arg, developer_dir, sdk_root, cwd, relative_ast_path,
-                    linked_binary, dsym_path, strip_debug_symbols,
                     toolchain_path, consumer);
   }
 
@@ -468,10 +438,12 @@ int main(int argc, char *argv[]) {
 
   // Check to see if we should postprocess with dsymutil.
   bool postprocess = false;
-  if ((!linked_binary.empty()) || (!dsym_path.empty())) {
-    if ((linked_binary.empty()) || (dsym_path.empty())) {
+  const char *linked_binary = getenv("DSYM_HINT_LINKED_BINARY");
+  const char *dsym_path_raw_value = getenv("DSYM_HINT_DSYM_PATH");
+  if (linked_binary != nullptr || dsym_path_raw_value != nullptr) {
+    if (linked_binary == nullptr || dsym_path_raw_value == nullptr) {
       const char *missing_dsym_flag;
-      if (linked_binary.empty()) {
+      if (linked_binary == nullptr) {
         missing_dsym_flag = "DSYM_HINT_LINKED_BINARY";
       } else {
         missing_dsym_flag = "DSYM_HINT_DSYM_PATH";
@@ -499,6 +471,7 @@ int main(int argc, char *argv[]) {
     return 0;
   }
 
+  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();
@@ -518,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<std::string> 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
@@ -64,9 +64,11 @@ def linking_test_suite(name):
         ],
         not_expected_argv = [
             "-g",
-            "DSYM_HINT_LINKED_BINARY",
             "-dead_strip",
         ],
+        not_expected_env_keys = [
+            "DSYM_HINT_LINKED_BINARY",
+        ],
         mnemonic = "ObjcLink",
         target_under_test = "//test/test_data:macos_binary",
     )
@@ -86,9 +88,11 @@ def linking_test_suite(name):
         ],
         not_expected_argv = [
             "-g",
-            "DSYM_HINT_LINKED_BINARY",
             "-dead_strip",
         ],
+        not_expected_env_keys = [
+            "DSYM_HINT_LINKED_BINARY",
+        ],
         mnemonic = "ObjcLink",
         target_under_test = "//test/test_data:ios_binary",
     )
@@ -182,6 +186,8 @@ def linking_test_suite(name):
         tags = [name],
         expected_argv = [
             "-g",
+        ],
+        expected_env_keys = [
             "DSYM_HINT_LINKED_BINARY",
         ],
         mnemonic = "ObjcLink",
@@ -191,7 +197,7 @@ def linking_test_suite(name):
     dsym_test(
         name = "{}_generate_cpp_dsym_test".format(name),
         tags = [name],
-        expected_argv = [
+        expected_env_keys = [
             "DSYM_HINT_LINKED_BINARY",
             "DSYM_HINT_DSYM_PATH",
         ],
diff --git 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(
