Skip to content

[Serialization] Prepare the infrastructure for the deserialization safety feature #62886

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

Conversation

xymus
Copy link
Contributor

@xymus xymus commented Jan 6, 2023

For context, deserialization safety is a new feature to avoid deserialization failures before they happen. The main idea is to mark decls that may refer to implementation details when serializing them to later skip deserializing them on the client side. This will avoid reading non-public decls and thus avoiding hitting XRefs errors from internal details.

See the skip-internal-details.swift test for an idea of internal details currently being read that could be skipped entirely.

Compared to deserialization recovery, the safety feature will skip deserializing problematic decls early, instead of recovering from failures hit later on. Note that the safety feature still relies on the recovery logic to pass up errors about decls that were unsafe to deserialize.

We still write unsafe decls for clients like LLDB that rely on reading internal details. Once they move to a different format, instead of marking decls as unsafe we could simply not write them out.

Deserialization safety relies on a record serialized before unsafe decls (only). This record has only an identifier used for debugging the following decl to skip. This identifier is intended to be temporary, it may add size to swiftmodule files but will help greatly to identify compiler bugs revealed by the safety feature. Considering that we may simply stop writing these decls down in the future, and thus cut down drastically on the swiftmodule file size, I believe this is a reasonable increase in size for the time being and optimizations are possible.


This PR sets up the basic infrastructure needed for the deserialization recovery feature. It introduces the flags to enable/disable it (keeping the feature off by default at the moment), the serialization record and error, and the core logic on the reader side when attempting to deserialize an unsafe decl.

Note that the feature is still toothless without the writer side logic.

Future PRs will introduce the logic on the writer side to determine if a decl is unsafe, add missing recovery logic identified by this feature, and update tests that were expecting internal details.

@xymus xymus requested review from bnbarham and nkcsgexi January 6, 2023 18:32
@xymus
Copy link
Contributor Author

xymus commented Jan 6, 2023

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Jan 6, 2023

CC @adrian-prantl This may be of interest to you. This new deserialization safety feature will remain disabled for LLDB where your extended recovery logic should still allow to recover from failures otherwise avoided by the safety feature.

xymus added 5 commits January 6, 2023 12:02
These prints can be enabled in an assert compiler with `-Xllvm
-debug-only=Serialization`.
Deserialization safety remains off by default at the moment.
The DESERIALIZATION_SAFETY record will be used by the deserialization
safety feature to identify decls that are unsafe to read so they can be
skipped.
…fe decl

Intro UnsafeDeserializationError and instanciate it when attempting to
deserialize a decl marked as unsafe by a DESERIALIZATION_SAFETY record.
@xymus xymus force-pushed the deser-safety-infra branch from c81bad2 to 294b5d0 Compare January 6, 2023 20:03
@xymus
Copy link
Contributor Author

xymus commented Jan 6, 2023

@swift-ci Please smoke test

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.

Makes a lot of sense to me. Thank you!

@xymus
Copy link
Contributor Author

xymus commented Jan 7, 2023

@swift-ci Please smoke test macOS

@xymus xymus merged commit 4e69160 into swiftlang:main Jan 7, 2023
@xymus xymus deleted the deser-safety-infra branch January 7, 2023 05:08
///
/// This is a staging flag; eventually it will be removed.
/// This flag should only be used in testing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems this isn't actually true since we need it enabled in order to have EnableDeserializationSafety work. Plus at the very least LLDB would like want this regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, thanks for pointing it out, it should say This flag should only be disabled in testing.. We always want this enabled unless we want to cause a crash.

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