Skip to content

[swiftinterface] Handle target variants the same as targets - #77156

Merged
drodriguez merged 1 commit into
swiftlang:mainfrom
drodriguez:swiftinterface-target-variant
Oct 22, 2024
Merged

[swiftinterface] Handle target variants the same as targets#77156
drodriguez merged 1 commit into
swiftlang:mainfrom
drodriguez:swiftinterface-target-variant

Conversation

@drodriguez

@drodriguez drodriguez commented Oct 22, 2024

Copy link
Copy Markdown
Contributor

Based on preliminary work from @rmaz.

The compilation arguments for a swiftinterface file are preprocessed to modify the -target argument to match the preferred target (which comes from the command line) in cases in which the sub-architecture differs, but it is compatible (for example using arm64e when arm64 is being compiled), but this was not done for the target variant, which ended up with mismatches on the sub-architecture used by the target and target variant, which fails an assert in assert toolchains.

Maybe solves rdar://135322077

@drodriguez

Copy link
Copy Markdown
Contributor Author

@swift-ci please test

@tshortli tshortli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking care of this! Out of curiosity, what assertion are you hitting without this fix? I thought the SILGen assertion I removed was the only one that was being triggered.

Comment thread lib/Serialization/SerializedModuleLoader.cpp Outdated
@drodriguez

Copy link
Copy Markdown
Contributor Author

Thanks for taking care of this! Out of curiosity, what assertion are you hitting without this fix? I thought the SILGen assertion I removed was the only one that was being triggered.

It was the assert in SILGenFunction::emitZipperedOSVersionRangeCheck that you removed in #76276. We found this in release/6.0 and I checked the main branch for the "source" of the problem, but I did not check if the assert was still there in main.

I will try to make the triple checks into a function/lambda and send a new version.

Based on preliminary work from @rhow.

The compilation arguments for a swiftinterface file are preprocessed to
modify the `-target` argument to match the preferred target (which comes
from the command line) in cases in which the sub-architecture differs,
but it is compatible (for example using `arm64e` when `arm64` is being
compiled), but this was not done for the target variant, which ended up
with mismatches on the sub-architecture used by the target and target
variant, which fails an assert in assert toolchains.
@drodriguez
drodriguez force-pushed the swiftinterface-target-variant branch from 9563f32 to 0b14bdc Compare October 22, 2024 17:40
@drodriguez

Copy link
Copy Markdown
Contributor Author

@swift-ci please test

@drodriguez
drodriguez enabled auto-merge (squash) October 22, 2024 17:52
@drodriguez
drodriguez merged commit 4e2fbe1 into swiftlang:main Oct 22, 2024
@drodriguez
drodriguez deleted the swiftinterface-target-variant branch October 25, 2024 17:21
nkcsgexi added a commit that referenced this pull request Dec 17, 2024
nkcsgexi added a commit that referenced this pull request Dec 18, 2024
Revert "[swiftinterface] Handle target variants the same as targets (#77156)"
tshortli added a commit to tshortli/swift that referenced this pull request Jan 2, 2025
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.
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