TimelyDataflow / abomonation

A mortifying serialization library for Rust
MIT License
322 stars 30 forks source link

[Based on #28] Getting alignment right #33

Open HadrienG2 opened 5 years ago

HadrienG2 commented 5 years ago

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

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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.