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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: use new abstraction to guarantee alignment #156

Merged
merged 3 commits into from Jun 22, 2023
Merged

Conversation

phip1611
Copy link
Collaborator

@phip1611 phip1611 commented Jun 22, 2023

Before this PR, both builder structs (HeaderBuilder, BootInformationBuilder) build their corresponding structures in a Vec<u8>. However, there is no guarantee of an 8-byte alignment in that case. As long as the allocator_api is not stable, I chose the following workaround:

Use a regular Vec<u8> with a capacity of expected_len + 7. This way, I can add zeroes at the beginning to the vec for alignment adjustments. A wrapper type returns a slice to the actual and aligned bytes of the corresponding structure, i.e, the wrapper type knows an offset between 0 and 7 where the structure actually starts.

The only problem with this type: I can't create a reliable unit test for the alignment offsetting magic. I created a local test run using the allocator_api and a "weird" (unaligned) pointer from the allocator and everything worked as expected. I tested the new code with miri and miri reported no allocation or alignment errors 馃コ

@phip1611 phip1611 force-pushed the builder-align-8 branch 3 times, most recently from c93ef70 to e9d04dd Compare June 22, 2023 11:08
@phip1611
Copy link
Collaborator Author

phip1611 commented Jun 22, 2023

@YtvwlD I'm curious. Did you encountered problems with the builder in your project due to alignment issues?

If the allocator "is friendly", you might never have encountered any problems. Unfortunately, this is very hard to unit-test without the allocator-api feature.

@phip1611
Copy link
Collaborator Author

Ah, interesting. On Windows, the allocator triggered one of my new assertions

https://github.com/rust-osdev/multiboot2/actions/runs/5344793550/jobs/9689626355?pr=156

@phip1611 phip1611 force-pushed the builder-align-8 branch 2 times, most recently from e3a5159 to b17b09c Compare June 22, 2023 11:43
@phip1611
Copy link
Collaborator Author

Strictly speaking, the Vec -> Box conversion is overkill. I could also keep the vector in the byte wrapper.

@phip1611 phip1611 force-pushed the builder-align-8 branch 2 times, most recently from 7fe8a13 to 4da4c36 Compare June 22, 2023 11:48
@phip1611 phip1611 merged commit c3a4e03 into main Jun 22, 2023
24 checks passed
@phip1611 phip1611 deleted the builder-align-8 branch June 22, 2023 15:08
@YtvwlD
Copy link
Contributor

YtvwlD commented Jun 22, 2023

I did not encounter any errors yet. But you're right: According to the spec the struct should be 8-byte aligned.

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.

None yet

2 participants