Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add swift.use_tmpdir_for_module_cache feature #456

Closed

Conversation

thii
Copy link
Member

@thii thii commented Jul 27, 2020

Add swift.use_tmpdir_for_module_cache feature

This adds a new feature, that can be enabled by passing
--features=swift.use_tmpdir_for_module_cache to the build
command.

This feature, when enabled, will makes the Swift compilation actions use
the shared Clang module cache path written to
/private/tmp/__build_bazel_rules_swift. This makes the embedded Clang
module breadcrumbs deterministic between Bazel instances, because they
are always embedded as absolute paths. Note that the use of this cache
is non-hermetic--the cached modules are not wiped between builds, and
won't be cleaned when invoking bazel clean; the user is responsible
for manually cleaning them.

Additionally, this can be used as a workaround for a bug in the Swift
compiler that causes the module breadcrumbs to be embedded even though the
-no-clang-module-breadcrumbs flag is passed
(https://bugs.swift.org/browse/SR-13275).

@thii thii force-pushed the deterministic_module_breadcrumbs branch from 0b119d4 to 76b0f84 Compare July 27, 2020 11:33
args.add(
"-module-cache-path",
paths.join(
"/tmp/build_bazel_rules_swift",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do a similar thing in our fork, but we set it to /private/var/tmp instead of /tmp, so it stays around after reboots. To support more than macOS it should be outputRoot instead.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is our patch, for example: 106add0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Are you interested in having this? I was bit skeptical since I found this doesn't solve all the cases.

Copy link
Collaborator

@brentleyjones brentleyjones Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the patch, since it made more .o files deterministic, which helps with Remote Build without the Bytes, but it's been awhile since I deep dived into it to see how much it helps.

So for now I would wait, either until the bug is fixed, or until I look again (which might be about a month, as I'm transitioning roles right now).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the non-deterministic bit often comes from ObjC or mixed source modules, I think this will have a better result on a Swift-only codebase.

I think I'll update this, and maybe rename the feature as it's a little misleading for what it does. I'm thinking of something like use_shared_module_cache or use_system_wide_module_cache or use_tmpdir_for_module_cache (the global one is used).

I'm not sure what "transitioning roles" means but congrats!

@thii
Copy link
Member Author

thii commented Jul 30, 2020

We did more testing and it looks like this alone is not enough. Seems we'll have to wait for the bug to be fixed.

@keith
Copy link
Member

keith commented Jul 30, 2020

What paths still make it in?

@thii
Copy link
Member Author

thii commented Jul 30, 2020

The pcm filenames are different, starting at some high level modules.

@thii
Copy link
Member Author

thii commented Jul 30, 2020

Something like:

@@ -9607,8 +9607,8 @@
               DW_AT_stmt_list  (0x00003854)
               DW_AT_comp_dir   ("bazel-out/ios-x86_64-min11.0-applebin_ios-ios_x86_64-dbg/bin/Talk")
               DW_AT_APPLE_optimized    (true)
-              DW_AT_GNU_dwo_id (0x63a64e8fed095117)
-              DW_AT_GNU_dwo_name       ("/tmp/build_bazel_rules_swift/bazel-out/ios-x86_64-min11.0-applebin_ios-ios_x86_64-dbg/bin/_swift_module_cache/3SHNZT125O13T/LINE-2KKB6W25CYN7B.pcm")
+              DW_AT_GNU_dwo_id (0xfbef8f5a83f3fbed)
+              DW_AT_GNU_dwo_name       ("/tmp/build_bazel_rules_swift/bazel-out/ios-x86_64-min11.0-applebin_ios-ios_x86_64-dbg/bin/_swift_module_cache/3SHNZT125O13T/LINE-1QK7C3WUA6TFP.pcm")

Cleaning the cached modules between builds doesn't resolve it.

@keith
Copy link
Member

keith commented Jul 30, 2020

@kastiglione what was this file name based on again?

@segiddins
Copy link
Contributor

I've found you can get a different DW_AT_GNU_dwo_id even when the DW_AT_GNU_dwo_name is constant -- would this patch also address that issue?

@segiddins
Copy link
Contributor

To quickly follow up on my question -- I think the answer is no. DW_AT_GNU_dwo_id might come regardless via -gsplit-dwarf, which is set in the crosstool -- inferences from rust-lang/rust#34902 (comment)

@thii
Copy link
Member Author

thii commented Aug 26, 2020

Nice finding. Would that mean we would have reproducible .os if we build with this feature and --features=-per_object_debug_info?

@segiddins
Copy link
Contributor

I'm not sure, I'd have to test it. I may have been misreading either the issue or the crosstool, so it was more an idea of a place to check than a statement of fact, sorry if I didn't make that clear

@thii
Copy link
Member Author

thii commented Aug 26, 2020

No need to apologize. I was too lazy to test it myself.

@thii
Copy link
Member Author

thii commented Aug 27, 2020

I just tested. You have to use your own --apple_crosstool_top in order to disable a crosstool feature. But the feature doesn't seem to have anything to do with Swift compilation—I still got unreproducible DW_AT_GNU_dwo_id.

@segiddins
Copy link
Contributor

darn :/ thanks for checking

@thii thii force-pushed the deterministic_module_breadcrumbs branch from 76b0f84 to 418d9f9 Compare December 14, 2020 06:50
@thii thii changed the title Add swift.deterministic_module_breadcrumbs feature Add swift.use_tmpdir_for_module_cache feature Dec 14, 2020
@thii thii force-pushed the deterministic_module_breadcrumbs branch 2 times, most recently from 6e102bb to 1010575 Compare December 15, 2020 05:28
This adds a new feature, that can be enabled by passing
`--features=swift.use_tmpdir_for_module_cache` to the build
command.

This feature, when enabled, will makes the Swift compilation actions use
the shared Clang module cache path written to
`/private/tmp/__build_bazel_rules_swift`. This makes the embedded Clang
module breadcrumbs deterministic between Bazel instances, because they
are always embedded as absolute paths. Note that the use of this cache
is non-hermetic--the cached modules are not wiped between builds, and
won't be cleaned when invoking `bazel clean`; the user is responsible
for manually cleaning them.

Additionally, this can be used as a workaround for a bug in the Swift
compiler that causes the module breadcrumbs to be embedded even though the
`-no-clang-module-breadcrumbs` flag is passed
(https://bugs.swift.org/browse/SR-13275).
@thii thii force-pushed the deterministic_module_breadcrumbs branch from 1010575 to 15f8628 Compare December 15, 2020 05:29
@thii
Copy link
Member Author

thii commented Dec 15, 2020

Rebased and added tests.

@thii thii closed this in #581 May 13, 2021
@thii thii deleted the deterministic_module_breadcrumbs branch May 13, 2021 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants