Skip to content
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

Use a standard header for metadata in all serialized modules #2747

Merged
merged 2 commits into from
Jan 14, 2022

Conversation

Amanieu
Copy link
Contributor

@Amanieu Amanieu commented Jan 7, 2022

This header includes an ABI version which will reject any serialized
modules from an incompatible version of Wasmer. Also, it replaces all of
the custom per-engine code for encoding the metadata length.

This header includes an ABI version which will reject any serialized
modules from an incompatible version of Wasmer. Also, it replaces all of
the custom per-engine code for encoding the metadata length.
@Amanieu Amanieu requested a review from syrusakbary as a code owner January 7, 2022 16:31
@Amanieu Amanieu requested a review from ptitSeb January 11, 2022 00:17
Copy link
Contributor

@ptitSeb ptitSeb left a comment

Choose a reason for hiding this comment

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

I guess all previously generated modules will be incompatible. That's kind-of a breaking change.

Comment on lines 49 to 50
const MAGIC_HEADER: &'static [u8; 22] = b"\0wasmer-universal\0\0\0\0\0";

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to preserve this. I see one thing as the metadata header, and the other is the file header.
Meaning: I think it will be useful to separate the file format from the metadata serialized version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metadata header itself already has a magic number and a length, so I felt that this was somewhat redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also in the case of the universal engine, there is only metadata and nothing else.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that the magic is only relevant for the file format (thus, for the universal engine).
We don't need magic in the metadata itself, since the metadata field name in the dynamic library is already the identifier.

Therefore, I think a better abstraction is to put the magic in the engine artifact and the rest (metadata version, length, ...) on the metadata as the PR is doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The metadata header needs to be 16 bytes since the serialized metadata is required to be 16-byte aligned. Since the length only uses 8 bytes, we have 8 bytes left that we might as well use for a magic.

Replacing the custom header of wasmer-universal with the standard MetadataHeader was done to make the code more maintainable by centralizing header-related code.

@Amanieu
Copy link
Contributor Author

Amanieu commented Jan 14, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 14, 2022

@bors bors bot merged commit 8c9a2a0 into master Jan 14, 2022
@bors bors bot deleted the metadata-header branch January 14, 2022 02:37
@webmaster128
Copy link
Contributor

I guess all previously generated modules will be incompatible. That's kind-of a breaking change.

#2781 discusses the stability of the module format. There also a module breaking change between Wasmer 2.0.0 and 2.1.0 is shown.

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.

4 participants