[std::span] {load,store}_{le,be}

[reneme]

Pull Request Dependencies

• Range-based mem_ops #3715

Description

This has two objectives:

1. range-based overloads for big/little endian helpers
   The lack of this has been annoying me for some time now. Using ranges facilitates maximum usage convenience while enabling opportunistic compile-time buffer size checks when the passed range has static length information (read: std::array or std::span<T, 42>). Otherwise runtime checks will be performed (BOTAN_ARG_CHECK).
2. Reduce the overload complexity
   ... by letting the compiler generate most of the generic code as constexpr. I didn't see any slow-downs (in the runtime of the test suite) after applying this.

Here's a godbolt (of a somewhat stripped-down version) to play around with that, if needed: https://godbolt.org/z/sxjhnbePo

Currently, this includes legacy (pointer-based) overloads of the existing helpers. Those just assert that the passed in pointers are big enough and wrap them into properly sized std::spans to forward the calls. In an ideal world we'd need to go through the code base and replace all those calls with statically checkable invocations.

[coveralls]

coverage: 92.053% (-0.006%) from 92.059%
when pulling 5e52e4b on Rohde-Schwarz:span/loadstor
into 3005ae6 on randombit:master.

[randombit]

In general this looks like a nice improvement, main concern would be if there are any performance implications. I would think not for the case where endian is known at compile time so we can make use of memcpy, and the targets where it's not known (or anyway assumed) are all relatively obscure, but I'd like to not introduce a regression if we don't have to.

[reneme]

Rebased after #3711 fixed the CI build.

[reneme]

Lets postpone this for a moment. I'm hoping that the iOS/Android update will bring more tools to make this easier.

[randombit]

I'm going to either update the emscripten build (#3720) or just disable the emscripten CI build, at which point we should be able to rely on having most of C++20.

[*] Outside of std::source_location, and probably 20 other things that we just haven't hit yet...

[reneme]

This patch was needed to explicitly convert size_t to uint64_t on 32-bit platforms.

[reneme]

Rebased after #3760 conflicted with the underlying #3715.

[reneme]

This will need a rebase. But let's review/merge #3715 first and get this done after.

[FAlbertDev]

I'll review this PR in the following days!

[FAlbertDev]

Nice! I'm looking forward to using these new interfaces! This will beautify many places, where load/store is used 😃

[reneme]

@FAlbertDev @randombit I rebased to master, and addressed the pending review comments (except the docstring remarks).

Request for Comment: Type inferring overloads for load_* and store_*

Explicitly note that I added a commit (b4c057e) that introduces convenience overloads for load_* and store_*, which infer their output type at compile time, if possible. Please hear me out:

I am envisioning those as freestanding helpers for all our byte-stream based APIs (most notably: Buffered_Computation, XOF, BufferSlicer, BufferStuffer, ...). We discussed introducing endian-conversion overloads on several occasions: e.g. for LMS, but also when introducing the XOF API). Instead of scattering endian-conversion overloads in all of such APIs, we could just compose them with those "smart" freestanding functions.

The resulting downstream code, would then look something along those lines:

void parse_some_tls_structure(std::span<const uint8_t> serialized) {
   BufferSlicer bs(serialized);
   const uint16_t tag = load_le(bs.take<2>());  // we could even use `auto` here...
   const uint16_t length = load_le(bs.take<2>());
   // ...
}

std::unique_ptr<XOF> prepare_xof_for_some_pqc_algo(std::span<const uint8_t> seed, uint32_t nonce) {
   auto xof = XOF::create_or_throw("SHAKE-256");
   xof->update(seed);
   xof->update(store_be(nonce)); // instead of adding an overload like `XOF::update_be()`
   return xof;
}

Comparison: without the type inference overloads

Without this load_*, we would need to explicitly declare the integer type (which is redundant, if the byte array length is known at compile time). Admittedly, that's a bit of template kink, though.

// assuming the BufferSlicer example
const uint16_t tag = load_le<uint16_t>(bs.take<2>());
//                           ~~~~~~~~

Similarly, with store_*, we avoid an explicit definition of a bytearray as sink, like so:

// assuming the XOF example
std::array<uint8_t, sizeof(nonce)> sink;
store_be(nonce, sink);
xof->update(sink);

I am arguing that these overloads provide maximum flexibility and convenience (while being statically checked for consistency by the compiler, as much as possible). And they allow easy composability with all modern (span or range-based) interfaces.

[FAlbertDev]

Yeah, I like the static return type deduction! We could discuss if we still want to add load/store members to the BufferSlicer and BufferStuffer. For now, however, I think this PR is ready to merge.

[reneme]

Thanks for the thorough review! @randombit no particular rush with this, but I'd love to have it in (after discussing #3707 (comment)) to get Frodo and LMS adapted to this. Kyber and Dilithium will follow.

[randombit]

Change itself looks fine, and a nice cleanup overall.

I do see a minor but persistent performance regression with Blowfish (about .2 cycles/byte slower) but for whatever reason that's the only algorithm I can find that seems slower, so not a problem.

[reneme]

🎉