Skip to content

[macCatalyst] Support generating macCatalyst object files #75261

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

Closed
wants to merge 1 commit into from

Conversation

drodriguez
Copy link
Contributor

This implements the missing support for macCatalyst in the open source version of Swift. It seems that years ago, the minimal support for macCatalyst was included in #29017, but except some fixes over the years, the work seemed to be blocked there, waiting for LLVM/Clang support that had not landed by the time and the open source toolchain cannot target macCatalyst.

  • Implement code to access to _stdlib_isOSVersionAtLeastOrVariantVersionAtLeast from the stdlib.
  • Implement code to choose isOSVersionAtLeast or isOSVersionAtLeastOrVariantVersionAtLeast depending on the command line being provided a target variant.
  • When an availability statement need to be emitted, pass the target variant version range in order to generate the right stdlib call.
  • Implement isOSVersionAtLeastOrVariantVersionAtLeast using the variant version information and forwarding both checks to isOSVersionAtLeast.
  • Set the LLVM Module with the target variant information, so the code is generated correctly.
  • Similar code for when the Clang importer is used.
  • Modify a test that, when used precompiled modules from Xcode, uses code compiled for macCatalyst and uses the isOSVersionAtLeastOrVariantVersionAtLeast instead of the previously expected isOSVersionAtLeast. Another option is using prefer-parseable, which uses the "expected" stdlib function.
  • Modify a test that was checking for availability in a fixed order even when the parameters in the command line were flipped. This test was probably never run in open source CI because it requires maccatalyst_support, which was not possible without these changes.

This implements the missing support for macCatalyst in the open source
version of Swift. It seems that years ago, the minimal support for
macCatalyst was included in #???, but the work was abandoned there, and
the open source toolchain cannot target macCatalyst.

- Implement code to access to
  `_stdlib_isOSVersionAtLeastOrVariantVersionAtLeast` from the stdlib.
- Implement code to choose `isOSVersionAtLeast` or
  `isOSVersionAtLeastOrVariantVersionAtLeast` depending on the command
  line being provided a target variant.
- When an availability statement need to be emitted, pass the target
  variant version range in order to generate the right stdlib call.
- Implement `isOSVersionAtLeastOrVariantVersionAtLeast` using the
  variant version information and forwarding both checks to
  `isOSVersionAtLeast`.
- Set the LLVM Module with the target variant information, so the code
  is generated correctly.
- Similar code for when the Clang importer is used.
- Modify a test that, when used precompiled modules from Xcode, uses
  code compiled for macCatalyst and uses the
  `isOSVersionAtLeastOrVariantVersionAtLeast` instead of the previously
  expected `isOSVersionAtLeast`. Another option is using
  `prefer-parseable`, which uses the "expected" stdlib function.
- Modify a test that was checking for availability in a fixed order even
  when the parameters in the command line were flipped. This test was
  probably never run in open source CI because it requires
  `maccatalyst_support`, which was not possible without these changes.
@drodriguez
Copy link
Contributor Author

@swift-ci please test

@drodriguez
Copy link
Contributor Author

For the people at Apple: I am pretty sure this code exists internally because Xcode is quite capable to compile Swift macCatalyst. I will be happy to abandon this PR if the internal code is contributed to the open source implementation, since it will avoid internal classes if this code ever lands in the repository. These are the changes I needed to get things working as I think they should work when using --maccatalyst in the build-script, but there's probably some code internally that I am missing, or the implementation could be better. Happy to help to open source those bits in whatever I can.

Copy link
Contributor

@tshortli tshortli left a comment

Choose a reason for hiding this comment

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

Hey @drodriguez, thanks for reminding us about this missing support. Let me investigate upstreaming the missing bits so that we can minimize divergence. I will update you here when I have more information.

@tshortli
Copy link
Contributor

tshortli commented Jul 18, 2024

This implements the missing support for macCatalyst in the open source version of Swift. It seems that years ago, the minimal support for macCatalyst was included in #29017, but except some fixes over the years, the work seemed to be blocked there, waiting for LLVM/Clang support that had not landed by the time and the open source toolchain cannot target macCatalyst.

I dug into this a bit and found that this is still blocked in general on several missing pieces in llvm/clang.

  • Implement isOSVersionAtLeastOrVariantVersionAtLeast using the variant version information and forwarding both checks to isOSVersionAtLeast.

In particular, this part of the PR doesn't produce correct behavior. isOSVersionAtLeastOrVariantVersionAtLeast needs to call a (currently unavailable) runtime function provided by compiler-rt that takes all 6 version arguments instead.

Finding a resolution to this will remain on my todo list but unfortunately it's going to take nontrivial effort to get it to parity and I may need help on the clang side. @drodriguez which pieces of missing macCatalyst support are blocking you the most? Maybe we can fill in some of the simpler pieces to get you unblocked sooner, while not immediately supporting runtime availability checks correctly.

@drodriguez
Copy link
Contributor Author

In particular, this part of the PR doesn't produce correct behavior. isOSVersionAtLeastOrVariantVersionAtLeast needs to call a (currently unavailable) runtime function provided by compiler-rt that takes all 6 version arguments instead.

I was quite sure that the implementation couldn't be that simple. I was just trying to reconstruct the function from the pieces that were already there. It is not the highest priority to have it in a working state, but obviously it would be great to have the actual implementation available.

@drodriguez which pieces of missing macCatalyst support are blocking you the most? Maybe we can fill in some of the simpler pieces to get you unblocked sooner, while not immediately supporting runtime availability checks correctly.

That would be great. I think the pieces in IRGen are the biggest missing pieces. Those are the ones I started with, but that led to a test breakage around isOSVersionAtLeastOrVariantVersionAtLeast, which I tried to fix, which led to the rest of the changes. If you can upstream those pieces in IRGen and maybe XFAIL the tests that relate to to the missing availability support, I think that would cover a lot.

I was looking at trying to implement what Egor ended up doing independently in #74994, and found out that the open source toolchain did not seem to support macCatalyst, so I was trying to fill the gaps. We have some interest in trying somethings out in macCatalyst, but without a working compiler we can target at macCatalyst it was not possible.

@tshortli
Copy link
Contributor

Here's a PR adding the majority of the missing support: #75432

@drodriguez
Copy link
Contributor Author

Thanks so much for all that work. I hope it at least reduces a little the internal patchset over at Apple.

Glad I was close in some cases and glad to see the actual code, because in many cases I would have break things so badly (funny that there actually was a _stdlib_isVariantOSVersionAtLeast and my laziness ended up using IsVariantOSVersionAtLeastDecl instead of the longer IsOSVersionAtLeastOrVariantVersionAtLeast).

Closing this one.

@drodriguez drodriguez closed this Jul 24, 2024
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.

2 participants