i wondered if we should output a directory for this or a flat file, but directory seems preferred. AFAICT the reason that the apple binary logic doesn't do that predates good tree artifact support. but today the apple logic also lipos the dsym binary if you do a multi-arch built, so it has some post processing that this will not too.

Directory is preferred as the .dSYM bundle can contain other resources too (like the UUID property list, see https://lldb.llvm.org/use/symbols.html).

I wondered if checking this feature was the right way to do this, and it seems fine, but it would also be equivalent to check the cpp fragment that the feature was based on

Would you prefer this instead (tested here and seems to work too)?

--- a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
@@ -597,7 +597,7 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
     # On macOS, if GENERATE_DSYM_FILE feature is enabled
     # then a .dSYM file will be built along with the executable.
     dsym_file = None
-    if cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "generate_dsym_file"):
+    if cpp_config.apple_generate_dsym:
         dsym_file = ctx.actions.declare_directory(
             "{name}.dSYM".format(
                 name = target_name,

Would you prefer this instead (tested here and seems to work too)?

I'm not sure which of these would be preferred. The latter seems more resilient to me, but the googlers who have to review can weigh in

@allevato could you review this change please?

C++ rules like cc_binary are @googlewalt 's domain, I'll defer to him.

@googlewalt could you please review these changes? Thank you!

i wondered if we should output a directory for this or a flat file, but directory seems preferred. AFAICT the reason that the apple binary logic doesn't do that predates good tree artifact support. but today the apple logic also lipos the dsym binary if you do a multi-arch built, so it has some post processing that this will not too.

Directory is preferred as the .dSYM bundle can contain other resources too (like the UUID property list, see https://lldb.llvm.org/use/symbols.html).

I missed this discussion before I reviewed. Currently the default behavior of both the internal and OSS toolchains is to output a file, so for consistency it seems this should be a file. Internally we have a bazel flag to control what the toolchain should output, but unfortunately that's internal only.

I wondered if checking this feature was the right way to do this, and it seems fine, but it would also be equivalent to check the cpp fragment that the feature was based on

Would you prefer this instead (tested here and seems to work too)?

--- a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
+++ b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
@@ -597,7 +597,7 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
     # On macOS, if GENERATE_DSYM_FILE feature is enabled
     # then a .dSYM file will be built along with the executable.
     dsym_file = None
-    if cc_common.is_enabled(feature_configuration = feature_configuration, feature_name = "generate_dsym_file"):
+    if cpp_config.apple_generate_dsym:
         dsym_file = ctx.actions.declare_directory(
             "{name}.dSYM".format(
                 name = target_name,

I'd prefer that since it's equivalent, shorter, and consistent with the objc linking code, but don't feel strongly.

I missed this discussion before I reviewed. Currently the default behavior of both the internal and OSS toolchains is to output a file, so for consistency it seems this should be a file. Internally we have a bazel flag to control what the toolchain should output, but unfortunately that's internal only.

I think the problem with this for cc_binary is that rules_apple does post processing https://github.com/bazelbuild/rules_apple/blob/14a8b6c05f71f3186d587c8b05c545a5d40ceae5/apple/internal/partials/debug_symbols.bzl#L172 to generate the Info.plist that ends up in the bundle, and with this approach no extra logic is required to generate the dsym with that structure (I don't actually know if that plist is required)

I don't think we can hardwire this code to support a directory. The functionality would be broken internally; it would not work with the apple_support toolchain; and it is inconsistent with the analogous code in objc linking.

If we want to support directory, I think it has to be in addition to supporting a file. One solution would be to add an internal flag for this -- I don't love it but at least it would work.

For this PR can we implement a file? If we need directory support we can continue discussion outside of it.

Thanks for the review, @googlewalt!

I don't think we can hardwire this code to support a directory. The functionality would be broken internally; it would not work with the apple_support toolchain; and it is inconsistent with the analogous code in objc linking.

I understand the concern. I'm currently working on adding proper support for dSYM bundles in apple_support here. The changes to wrapped_clang are backwards compatible with flat dSYMs. Since Bazel hasn’t previously supported dSYM bundles fully, could you clarify what internal functionality would break? I’d be happy to help coordinate changes in rules_apple and elsewhere to ensure consistent support.

For this PR can we implement a file? If we need directory support we can continue discussion outside of it.

I’d prefer to focus on proper support for dSYM bundles in this PR, rather than implementing flat dSYMs. Beyond embedding plists, we also rely on bundles to include Python scripts that LLDB loads automatically (for example https://dmaclach.medium.com/kernel-debugging-on-apple-silicon-ff5aa76c4429 and https://github.com/apple/darwin-xnu/blob/main/tools/lldbmacros/Makefile). These features and usecases don’t work with flat dSYM files.

I’d love to discuss how we can move forward with robust support for bundled dSYMs — happy to collaborate on that!

@googlewalt any thoughts on this? What is your suggestion to move forward?

I see. I need to make some changes so that this is compatible to our toolchain internally, but your proposal looks reasonable.

@iancha1992 would you be able to merge this PR? Thank you!

I'm getting it merged; might take a day or two to find someone to review it.