Skip to content

Frontend: Fix -target-variant subarch normalization #78413

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

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Jan 2, 2025

In #77156, normalization was introduced for -target-variant triples. That PR also caused -target-variant arguments to be inherited from the main compilation options whenever building dependency modules from their interfaces, which is incorrect. The -target-variant option must only be specified when compiling a "zippered" module, but the dependencies of zippered modules are not necessarily zippered themselves and indiscriminately propagating the option can cause miscompilation.

The new, more targeted approach to normalizing arm64e triples simply uses the arch and subarch of the -target argument of the main compile to decide whether the subarch of both the -target and -target-variant arguments of a dependency need adjustment.

Resolves rdar://135322077 and rdar://141640919.

In swiftlang#77156, normalization was introduced
for -target-variant triples. That PR also caused -target-variant arguments to
be inherited from the main compilation options whenever building dependency
modules from their interfaces, which is incorrect. The -target-variant option
must only be specified when compiling a "zippered" module, but the dependencies
of zippered modules are not necessarily zippered themselves and
indiscriminantly propagating the option can cause miscompilation.

The new, more targeted approach to normalizing arm64e triples simply uses the
arch and subarch of the -target argument of the main compile to decide whether
the subarch of both the -target and -target-variant arguments of a dependency
need adjustment.

Resolves rdar://135322077 and rdar://141640919.
@tshortli
Copy link
Contributor Author

tshortli commented Jan 2, 2025

@swift-ci please test

@tshortli
Copy link
Contributor Author

tshortli commented Jan 2, 2025

@swift-ci please build toolchain macOS

Copy link
Contributor

@nkcsgexi nkcsgexi left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the root cause Allan!

@tshortli tshortli merged commit f6bf596 into swiftlang:main Jan 3, 2025
6 checks passed
@tshortli tshortli deleted the target-variant-normalization branch January 3, 2025 07:23
@drodriguez
Copy link
Contributor

Thanks for the fix, Allan, and sorry for the breakage.

One thing I noticed while working on the original problem is that most of the macCatalyst tests do not run in swift.org open source CI and it is quite easy to break them, and some of them are broken already (the ones in this PR should run because they do not require macCatalyst at all). I tried providing a preset in #77386 and fix some of the tests in #77506 (which partial success). I hope Apple has some kind of testing for those internally, but can we have a look at least to having a preset so we can test in the open source CI (and maybe fix those broken tests)?.

@tshortli
Copy link
Contributor Author

tshortli commented Jan 7, 2025

I don't think this breakage would have been caught by a macCatalyst preset because we didn't have coverage for this issue, even in the tests with REQUIRES: maccatalyst_support (this issue wasn't identified by a CI bot). I agree it would be nice to have macCatalyst coverage in public CI, though. Maybe @shahmishal can speak to what it would take to get that set up. My interim mitigation for the problem has been to remove REQUIRES: maccatalyst_support from as many tests as I can, and I did remove it from quite a few over this summer.

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.

3 participants