Skip to content

Upstream some binary-compatibility logic #74821

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 10, 2024
Merged

Upstream some binary-compatibility logic #74821

merged 3 commits into from
Jul 10, 2024

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Jun 28, 2024

A couple of compatibility checks to preserve behavior for existing binaries on Apple platforms.

@tbkka tbkka requested review from mikeash and al45tair as code owners June 28, 2024 17:22
@tbkka
Copy link
Contributor Author

tbkka commented Jun 28, 2024

@swift-ci Please test

@tbkka tbkka requested a review from lorentey June 28, 2024 17:23
@tbkka tbkka requested a review from a team as a code owner June 29, 2024 00:24
@tbkka
Copy link
Contributor Author

tbkka commented Jun 29, 2024

@swift-ci Please test

@tbkka
Copy link
Contributor Author

tbkka commented Jul 1, 2024

@swift-ci Please test macOS Platform

@tbkka
Copy link
Contributor Author

tbkka commented Jul 2, 2024

@swift-ci Please test macOS platform

@tbkka
Copy link
Contributor Author

tbkka commented Jul 8, 2024

@swift-ci Please test macOS Platform

tbkka added 2 commits July 9, 2024 13:30
@tbkka
Upstream some binary-compatibility logic
0ab1b16
@tbkka
Verify that hash/equality behavior is all consistent
48eb993 
It's not obvious that we can check that hash/equality behavior
is entirely correct, since there are two very different behaviors
which depend on environmental factors that are not easy to test for.

But we can do a quick probe to see whether the current environment
seems to be offering the legacy or non-legacy behavior and then
carefully verify that everything else is consistent with our initial
probe.

This gives us confidence that at least we're not getting inconsistent
behavior.
@tbkka tbkka force-pushed the tbkka-bincompat branch from e8cdd74 to 48eb993 Compare July 9, 2024 20:30
@tbkka
Update executor test
4470ed7 
The test previously checked for a message that is no longer
emitted.  Drop it.
@tbkka tbkka requested a review from ktoso as a code owner July 9, 2024 20:32
@tbkka
Copy link
Contributor Author

tbkka commented Jul 9, 2024

@ktoso -- It looks like I needed a small test adjustment in order to upstream the bincompat check for executor assertion behavior. Any concerns?

@tbkka
Copy link
Contributor Author

tbkka commented Jul 9, 2024

@swift-ci Please test

tbkka
tbkka commented Jul 9, 2024
View reviewed changes
test/Distributed/Runtime/distributed_actor_custom_executor_availability.swift
@@ -38,7 +38,7 @@ import Distributed
}

tests.test("5.7 actor, 5.9 executor property => no custom executor") {
expectCrashLater(withMessage: "Incorrect actor executor assumption; Expected MainActor executor")
expectCrashLater()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new semantics generate the crash in the swift_task_isCurrentExecutorImpl function, which occurs before the message is emitted.

@tbkka tbkka merged commit 55d4a56 into swiftlang:main Jul 10, 2024
5 checks passed
@tbkka tbkka deleted the tbkka-bincompat branch July 10, 2024 21:18
@bnbarham bnbarham mentioned this pull request Oct 3, 2024
Merged
@Kyle-Ye
Copy link
Contributor

Kyle-Ye commented Apr 9, 2025 •
edited
Loading

Some Notes on this PR: @tbkka

  1. This represents a behavior-breaking change when building with the new Xcode 16.0+/Swift
    toolchain, which could cause some applications to crash without notice. While I support the new behavior, it would be beneficial to have this information documented in the vendor's official release notes (e.g., Xcode 16 release notes) so that the downstream developer can adapt it in advance preparing for such change.

  2. Regarding the line: const dyld_build_version_t fall_2024_os_versions = {0xffffffff, 0x007e80000};

I recall that dyld_build_version_t is defined as a struct containing a uint32 platform and a uint32 version. Therefore, I believe the latter value should be 0x07e80000 instead of 0x007e80000, as the current value exceeds 32 bits literal. Though most compilers will likely truncate this value correctly, it would be more accurate to use the proper 32-bit representation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeash mikeash mikeash approved these changes

@al45tair al45tair Awaiting requested review from al45tair al45tair is a code owner

@lorentey lorentey Awaiting requested review from lorentey

@ktoso ktoso Awaiting requested review from ktoso ktoso is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tbkka @Kyle-Ye @mikeash