Fix internal alignment in frunk heterogeneous lists
#929
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ma2bd
reviewed
Aug 2, 2023
ma2bd
reviewed
Aug 2, 2023
jvff
force-pushed
the
fix-hlist-internal-alignment
branch
from
762f39b to
039a590
Compare
jvff
force-pushed
the
fix-hlist-internal-alignment
branch
from
039a590 to
d73bee9
Compare
ma2bd
approved these changes
Aug 3, 2023
| /// An address for a location in a guest WebAssembly module's memory. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub struct GuestPointer(pub(crate) u32); | ||
|
|
||
| impl GuestPointer { | ||
| /// Returns a new address that's the current address advanced to add padding to ensure it's | ||
| /// aligned to the `alignment` byte boundary. | ||
| pub fn aligned_at(&self, alignment: u32) -> Self { | ||
| pub const fn aligned_at(&self, alignment: u32) -> Self { |
ma2bd
reviewed
Aug 3, 2023
| /// An address for a location in a guest WebAssembly module's memory. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub struct GuestPointer(pub(crate) u32); | ||
|
|
||
| impl GuestPointer { | ||
| /// Returns a new address that's the current address advanced to add padding to ensure it's | ||
| /// aligned to the `alignment` byte boundary. | ||
| pub fn aligned_at(&self, alignment: u32) -> Self { | ||
| pub const fn aligned_at(&self, alignment: u32) -> Self { | ||
| let padding = (-(self.0 as i32) & (alignment as i32 - 1)) as u32; |
There was a problem hiding this comment.
Maybe comment something like
// Compute `-self.0` modulo `alignment`
There was a problem hiding this comment.
Done, and linked to Wikipedia where I first found the formula 😆
ma2bd
reviewed
Aug 3, 2023
|
|
||
| /// Test roundtrip of a heterogeneous list that doesn't need any internal padding. | ||
| #[test] | ||
| fn hlist_without_padding() { |
There was a problem hiding this comment.
Maybe assert the SIZE of the WitTypes?
There was a problem hiding this comment.
Good idea. Done inside the test_memory_roundtrip function.
Allow them to be used in the `const` context that calculates the size of a heterogeneous list.
Link to the Wikipedia source.
Ensure that the non-trivial padding calculation is correct.
Because heterogeneous lists are recursive types, some special handling is needed. Alignment between internal elements must be calculated separately, because otherwise the alignment for the whole list (the tail) is used, which is not the same as the alignment as the next element. The same is true when reading and writing to memory, because the alignment between each element must be considered, and not the alignment for all the remaining elements.
There's only one place where padding is needed.
Make sure that they can be stored in memory and read from memory, and that they can be lowered into its flat layout and lifted back from it. Ensure that their internal element alignment is properly respected.
jvff
force-pushed
the
fix-hlist-internal-alignment
branch
from
d73bee9 to
6957f3e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The
WitType,WitLoadandWitStoreimplementations forfrunkheterogeneous lists (HConsandHNiltypes) weren't respecting the alignment of its elements when reading and writing to memory. This meant that they didn't correctly implement WIT's canonical ABI specification.Proposal
Respecting the alignment for its elements is not simple because the lists are represented by recursive types. The
HConstype for example can check the alignment of its element (the "head") or of the rest of the list (the "tail"). However, the alignment of a list is defined as the largest alignment of its elements. So using the tail's alignment is not the correct solution. Instead, the alignment of the first element of the tail must be obtained.Also, in order to calculate the size of the list, internal padding must be included. The padding is calculated based on the alignment of each element. The solution is to calculate the padding using the size up until the current element, and the alignment of the next element. Unfortunately, it's not possible to write a
const fnto calculate the size this way, so instead a workaround must be used. Since the maximum alignment is expected to be 8 bytes (for the largest flat types,i64andf64), we can enumerate the size's offset inside that 8-byte window, and implement the size calculation considering each case.A helper
SizeCalculationtrait was added that is used internally to calculate the size and to obtain the first element of the tail.Test Plan
Two unit tests were written to check the roundtrip to memory and to a flat layout of heterogeneous lists that require internal padding and that don't. The test with the list that requires padding would fail before the fix.
Links
This is unexpected work part of #906
Release Plan
Reviewer Checklist