Skip to content

Disable zippered catalyst builds again #62882

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 2 commits into from
Jan 6, 2023

Conversation

etcwilde
Copy link
Member

@etcwilde etcwilde commented Jan 6, 2023

This patch does two things. One, zippers the Onone support library that I missed in the first pass.
Two, disables the catalyst build again. The open-source SwiftC appears to be missing something for catalyst, which would result in a miscompile situration.

The failing test:
SILGen/availability_query_maccatalyst_zippered_canonical_versions.swift

if #available(OSX 10.16, iOS 51.1.2, *) {
}

The expectation is that we check for both macOS and iOS version numbers in a catalyst build;

// CHECK: [[MACOS_MAJOR:%.*]] = integer_literal $Builtin.Word, 10
// CHECK: [[MACOS_MINOR:%.*]] = integer_literal $Builtin.Word, 16
// CHECK: [[MACOS_PATCH:%.*]] = integer_literal $Builtin.Word, 0
// CHECK: [[IOS_MAJOR:%.*]] = integer_literal $Builtin.Word, 51
// CHECK: [[IOS_MINOR:%.*]] = integer_literal $Builtin.Word, 1
// CHECK: [[IOS_PATCH:%.*]] = integer_literal $Builtin.Word, 2
// CHECK: [[FUNC:%.*]] = function_ref @$ss042_stdlib_isOSVersionAtLeastOrVariantVersiondE0yBi1_Bw_BwBwBwBwBwtF : $@convention(thin) (Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word) -> Builtin.Int1
// CHECK: [[QUERY_RESULT:%.*]] = apply [[FUNC]]([[MACOS_MAJOR]], [[MACOS_MINOR]], [[MACOS_PATCH]], [[IOS_MAJOR]], [[IOS_MINOR]], [[IOS_PATCH]]) : $@convention(thin) (Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word, Builtin.Word) -> Builtin.Int1

Currently, the OSS Swiftc is only emitting checks for macOS. I don't have time at the moment to dig further than that.

The build system should be in a place to handle catalyst builds, but it looks like the compiler is not.

I missed zippering the Onone support library resulting in some test
failures. Perhaps CMake should just add this to all of the runtime
libraries when targeting the mac?
While clang and the build-system can now handle generating
Catalysit-supporting executables, the OSS swift-frontend is missing
something.

This is failing in:
  SILGen/availability_query_maccatalyst_zippered_canonical_versions.swift

The compiler is only emitting the availability check-info for the macOS
target, not the iOS target.

e.g. for the following snippet

```
if #available(OSX 10.16, iOS 51.1.2, *) {
}
```

We end up emitting the literal checks for

integer_literal $Builtin.Word, 10
integer_literal $Builtin.Word, 16
integer_literal $Builtin.Word, 0

but we're missing

integer_literal $Builtin.Word, 51
integer_literal $Builtin.Word, 1
integer_literal $Builtin.Word, 2

This would be a most unfortunate miscompile.
@etcwilde etcwilde requested review from xedin and shahmishal January 6, 2023 17:49
@etcwilde
Copy link
Member Author

etcwilde commented Jan 6, 2023

@swift-ci please test

@etcwilde
Copy link
Member Author

etcwilde commented Jan 6, 2023

CC: @mikepinkerton

I gave turning on the zippered builds a shot. I don't have time at the moment to dig through the miscompile. Just letting you know that we're turning it back off for the time being.

@xedin xedin merged commit 9bbacdf into swiftlang:main Jan 6, 2023
@etcwilde etcwilde deleted the ewilde/fix-zipper branch April 2, 2023 05:54
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