Skip to content

Non-package module should load swiftinterface instead of binary module with PackageCMO. #76338

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

elsh
Copy link
Contributor

@elsh elsh commented Sep 9, 2024

If the binary module optimized with Package CMO is loaded for a non-package client module, the client module does not understand the serialized content of the optimized binary module because it contains attributes and instructions specific to the optimization, i.e. [serialized_for_package] definition that contains operations on non-loadable types that are normally not allowed. This can cause an assert or a crash in SIL verifier.

This PR checks whether the optimized binary module and the client module are in the same package, and require loading swiftinterface module if not.

Resolves rdar://134584629

@elsh
Copy link
Contributor Author

elsh commented Sep 9, 2024

@swift-ci smoke test

@elsh elsh force-pushed the elsh/pkg-load-interface-external-client branch from e8d9227 to a483cdb Compare September 9, 2024 16:26
@elsh
Copy link
Contributor Author

elsh commented Sep 9, 2024

@swift-ci smoke test

@xymus
Copy link
Contributor

xymus commented Sep 9, 2024

Can you move the MODULE_PACKAGE_NAME and SERIALIZE_PACKAGE_ENABLED fields from the options_block to the control_block as part of this PR? The control_block should have all the core information to tell if a module can be loaded or not, whereas the options_block is for additional information that is not always read depending on the client. This change should also streamline the logic as there's existing logic to reject the module. For an example you can take a look at the CHANNEL field and how it’s used to throw an ChannelIncompatible error.

The alternative would be like IS_TESTABLE, allowing to load the swiftmodule and report an error on the import. That's more appropriate in the case of a misconfiguration, the control_block approach is better for silently rejecting a swiftmodule file to prefer the swiftinterface.

@elsh
Copy link
Contributor Author

elsh commented Sep 12, 2024

The correct approach is to load the same optimized binary module for use by external clients of the package. Therefore, I prefer to keep MODULE_PACKAGE_NAME and SERIALIZE_PACKAGE_ENABLED within the options block, as it wouldn't make sense to move them to the control block and then back again. I've also updated the code comments to reflect this. Regarding diagnostics, serialization::Status::PackageMismatch has been added, along with the RebuildInfo::emitRemarks.

@elsh
Copy link
Contributor Author

elsh commented Sep 12, 2024

@swift-ci smoke test

binary module with PackageCMO enabled.

If the binary module optimized with Package CMO is loaded for
a non-package client module, the client module does not
understand the serialized content of the optimized binary
module because it contains attributes and instructions specific
to the optimization, i.e. [serialized_for_package] definition
that contains operations on non-loadable types that are normally
not allowed. This can cause an assert or a crash in SIL verifier.

This PR checks whether the optimized binary module and the client module
are in the same package, and require loading swiftinterface module
if not.

Resolves rdar://134584629
@elsh elsh force-pushed the elsh/pkg-load-interface-external-client branch from f1711c0 to 1261f2b Compare September 13, 2024 11:20
@elsh
Copy link
Contributor Author

elsh commented Sep 13, 2024

@swift-ci smoke test

@elsh
Copy link
Contributor Author

elsh commented Sep 13, 2024

After further consideration, it seems more reasonable to implement the proper long-term fix stated above instead of relying on this interim solution, especially since there's already a workaround: pass -Xfrontend -module-load-mode -Xfrontend prefer-interface. Thus will be closing this PR.

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