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

[Based on #28] Getting alignment right #33

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

HadrienG2
Copy link

@HadrienG2 HadrienG2 commented Oct 28, 2019

This allows Abomonation to correctly align serialized data by providing...

  • An encode() implementation that inserts padding so that all output data can be well-aligned if the bytes are aligned on a sufficiently strong boundary (specified by Abomonation::alignment()).
  • A decode() implementation that handles said padding correctly if the input data is suitably aligned (otherwise UB may occur).
  • Supporting tools for reallocating bytes with proper alignment for decoding if such alignment cannot be guaranteed by the data source

Other noteworthy changes include extent() going away (because alignment-related padding makes a type's extent dependent on the context in which it is entombed) and unsafe_abomonate becoming even more messy, raising the question of when it should go away for good.

At the documentation level, padding bytes related UB was significantly clarified.

For convenience reasons, this PR is based on #22, #24, #25, #26 and #28. Reviewing these PRs first is recommended, but if you want to review this one in isolation, please consider doing so commit-by-commit in order to get a clean diff.

Fixes #23.

@HadrienG2
Copy link
Author

@frankmcsherry So, this PR is getting into a pretty nice shape now. It would be useful if you could have a look at it and comment on the general design direction at some point.

In the end, I gave up on the idea of automatically reallocating decode()'s input data if it is misaligned, as I proposed in #23, because...

  1. Owned Abomonated data has ergonomic issues (in the presence of references) and should therefore not be something that we push onto users by default. Which we are forced to do in the decode API in the above design.
  2. decode() can already fail in so many ways due to broken input data invariants that adding an extra alignment invariant does not seem so terrible after all.

@HadrienG2
Copy link
Author

HadrienG2 commented Nov 11, 2019

One safer alternative to the current design where passing unaligned bytes to decode is UB (and is checked only in debug build) would be to have decode() required AlignedBytes as input. But that might be perceived to be excessively intrusive by performance-conscious people who have other ways of guaranteeing data alignment...

EDIT: ...especially as it would interfere somewhat badly with Abomonated's current design, which accepts any kind of bytes container.

@HadrienG2
Copy link
Author

HadrienG2 commented Nov 11, 2019

Alright, I squashed my commit mess a bit to make it easier for you to review. Also, I think I've found a way to make AlignedBytes and Abomonated play nice with each other, will try that via further commits...

EDIT: Nope, I don't think I can make it work without an unacceptable degree of type-level complexity.

@HadrienG2 HadrienG2 changed the title [WIP] [Based on #28] Getting alignment right [Based on #28] Getting alignment right Nov 12, 2019
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.

Can the pointer alignment situation be improved?
1 participant