Skip to content

Serialization: Stop reading control block early on version mismatch and fix current control block ordering #73828

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.

Sign up for GitHub

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 4 commits into from
Jun 5, 2024
Merged

Serialization: Stop reading control block early on version mismatch and fix current control block ordering #73828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9779806
Serialization: Stop reading the control block if the format version d…
xymus May 22, 2024
76e7fa9
Serialization: Move the distribution channel to the end of the contro…
xymus May 22, 2024
998a40a
Serialization: Move SDK version to the end of the control block enum
xymus May 22, 2024
ab6a701
Merge branch 'main' into serial-fix-control-block
xymus Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/Serialization/ModuleFileSharedCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,12 @@ static ValidationInfo validateControlBlock(
LLVM_FALLTHROUGH;
case 3:
result.shortVersion = blobData.slice(0, scratch[2]);

// If the format version doesn't match, give up after also getting the
// compiler version. This provides better diagnostics.
if (result.status != Status::Valid)
return result;

Comment on lines +337 to +342
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in the other PR, not sure about how any of this work, I just want to understand the consequences of this change. Wouldn't this make working with a compiler from main impossible to use against files compiled with previous compilers? That would make using .swiftmodule files distributed by Xcode not valid, wouldn't it? Before this I think old files were still mostly compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't change the current behavior. I would not expect a main compiler to be able to use swiftmodules built by any compiler older than a week or so. If that swiftmodule has a different serialization format it would have been rejected already.

The main content of swiftmodule files have very limited compatibility as they are designed to be a local cache. In theory they should only be read by the exact same compiler that wrote them, and in the exact same context. That's why any distributed modules should use swiftinterfaces. And the only swiftmodules shipped with Xcode are only the prebuilt cache, they can all be rebuilt from swiftinterfaces in a similar manner by newer compilers.

So any swiftmodule with a different serialization format version are rejected. swiftmodule files are also rejected between released compilers with a different tag. We leave some room for dev compilers, just to avoid rebuilding the stdlib all the time during development.

As you pointed out, the control block should be compatible with different compilers. But as soon as we know it should be rejected it's safe to skip reading the rest of the information.

LLVM_FALLTHROUGH;
case 2:
case 1:
Expand Down
27 changes: 13 additions & 14 deletions lib/Serialization/ModuleFormat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,7 @@ const uint16_t SWIFTMODULE_VERSION_MAJOR = 0;
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
/// Don't worry about adhering to the 80-column limit for this line.
const uint16_t SWIFTMODULE_VERSION_MINOR =
875; // Add package field to SerializedKind_t
const uint16_t SWIFTMODULE_VERSION_MINOR = 876; // Reorder control block enum

/// A standard hash seed used for all string hashes in a serialized module.
///
Expand Down Expand Up @@ -862,11 +861,11 @@ namespace control_block {
MODULE_NAME,
TARGET,
SDK_NAME,
SDK_VERSION,
REVISION,
CHANNEL,
IS_OSSA,
ALLOWABLE_CLIENT_NAME,
CHANNEL,
SDK_VERSION,
};

using MetadataLayout = BCRecordLayout<
Expand Down Expand Up @@ -897,21 +896,11 @@ namespace control_block {
BCBlob
>;

using SDKVersionLayout = BCRecordLayout<
SDK_VERSION,
BCBlob
>;

using RevisionLayout = BCRecordLayout<
REVISION,
BCBlob
>;

using ChannelLayout = BCRecordLayout<
CHANNEL,
BCBlob
>;

using IsOSSALayout = BCRecordLayout<
IS_OSSA,
BCFixed<1>
Expand All @@ -921,6 +910,16 @@ namespace control_block {
ALLOWABLE_CLIENT_NAME,
BCBlob
>;

using ChannelLayout = BCRecordLayout<
CHANNEL,
BCBlob
>;

using SDKVersionLayout = BCRecordLayout<
SDK_VERSION,
BCBlob
>;
}

/// The record types within the options block (a sub-block of the control
Expand Down
4 changes: 2 additions & 2 deletions lib/Serialization/Serialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,11 +834,11 @@ void Serializer::writeBlockInfoBlock() {
BLOCK_RECORD(control_block, MODULE_NAME);
BLOCK_RECORD(control_block, TARGET);
BLOCK_RECORD(control_block, SDK_NAME);
BLOCK_RECORD(control_block, SDK_VERSION);
BLOCK_RECORD(control_block, REVISION);
BLOCK_RECORD(control_block, CHANNEL);
BLOCK_RECORD(control_block, IS_OSSA);
BLOCK_RECORD(control_block, ALLOWABLE_CLIENT_NAME);
BLOCK_RECORD(control_block, CHANNEL);
BLOCK_RECORD(control_block, SDK_VERSION);

BLOCK(OPTIONS_BLOCK);
BLOCK_RECORD(options_block, SDK_PATH);
Expand Down