-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Conversation
@swift-ci Please smoke test |
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. |
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.
c81bad2
to
294b5d0
Compare
@swift-ci Please smoke test |
There was a problem hiding this 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!
@swift-ci Please smoke test macOS |
/// | ||
/// This is a staging flag; eventually it will be removed. | ||
/// This flag should only be used in testing. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.