Skip to content

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 7, 2025

This corrects how we were dealing with dispatch thunks -- mostly be
removing a lot of special casing we did but doesn't seem necessary and
instead we correct and emit all the necessary information int TBD.

This builds on #74935 by further refining how we fixed that issue, and adds more regression tests. It also removes a load of special casing of distributed thunks in library evolution mode, which is great.

Resolves and adds regression test for for rdar://145292018

This is also a more proper fix to the previously resolved but in a not-great-way which caused other issues:

  • refs rdar://128284016
  • refs rdar://128310903

@ktoso ktoso marked this pull request as draft April 7, 2025 12:04
@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Apr 17, 2025
@ktoso ktoso force-pushed the wip-regression-test-for-b-da-bad-access branch 2 times, most recently from e1ca212 to 4fcbfbe Compare April 18, 2025 02:19
@ktoso ktoso marked this pull request as ready for review April 18, 2025 02:19
@ktoso ktoso requested review from jckarter and rjmccall as code owners April 18, 2025 02:19
…ust work

This corrects how we were dealing with dispatch thunks -- mostly be
removing a lot of special casing we did but doesn't seem necessary and
instead we correct and emit all the necessary information int TBD.

This builds on  swiftlang#74935 by further refining how we fixed that issue, and adds more regression tests. It also removes a load of special casing of distributed thunks in library evolution mode, which is great.

Resolves and adds regression test for for rdar://145292018

This is also a more proper fix to the previously resolved but in a not-great-way which caused other issues:
- resolves rdar://128284016
- resolves rdar://128310903
@ktoso ktoso force-pushed the wip-regression-test-for-b-da-bad-access branch from 4fcbfbe to 957a95b Compare April 18, 2025 02:21
@ktoso
Copy link
Contributor Author

ktoso commented Apr 18, 2025

Figured out a solution and even fixed other TBD issues along the way, and past TBD issues in a cleaner way. Will want to verify this in a project as well to make sure we don't break anyone with this but it looks solid -- way less special casing!

@ktoso ktoso requested a review from xedin April 18, 2025 02:23
@ktoso
Copy link
Contributor Author

ktoso commented Apr 18, 2025

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 18, 2025

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Apr 18, 2025

@swift-ci please build toolchain macOS

@xedin
Copy link
Contributor

xedin commented Apr 18, 2025

@swift-ci please test source compatibility

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM! I left a few comments about the test inline but nothing blocking.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 21, 2025

Hmm, build issue on simulator run, let me restrict this to macos and linux

:0: warning: using sysroot for 'iPhoneSimulator' but targeting 'MacOSX'
:0: warning: using sysroot for 'iPhoneSimulator' but targeting 'MacOSX'
ld: building for 'macOS', but linking in dylib (/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator.sdk/usr/lib/libobjc.A.tbd) built for 'iOS-simulator'
:0: error: link command failed with exit code 1 (use -v to see invocation)

@ktoso
Copy link
Contributor Author

ktoso commented Apr 21, 2025

@swift-ci please test

@ktoso ktoso merged commit d788713 into swiftlang:main Apr 21, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants