3
\$\begingroup\$

I see three complexities in accessing (reading/writing) integer field in network packet.

  1. Handle endianness. Integer in network packet is big-endian (BE). The host may be either big-endian or little-endian (LE).
  2. The integer may have a bit width that is not a multiple of 8. E.g., IPv4 fragment offset has 13 bits.
  3. The integer may not align to byte boundary.

I authored a group of functions to help with the task. I'm not quite confident in this and seeking comments in terms of

  1. interface design (you may find my current one odd),
  2. correctness,
  3. performance.

Pertaining to correctness, I know my code fails for cases like "4 bits + 7 bytes + 4 bits". It's a legit input for the payload fits into a 64-bit uint. I cannot come up with an algorithm that handles this without sacrificing too much performance. So I decided not to support it.

As an example, for IPv4 fragment offset, we have len = 2, hishf = 3, and loshf = 0.

#include <cstdint>
#include <cstring>

#include <bit>
#include <ostream>
#include <format>

namespace net {

using byte_t = std::uint8_t;
using uint_t = std::uint64_t;

static_assert(
    std::endian::native == std::endian::little ||
    std::endian::native == std::endian::big
);

// Network byte order (big-endian)
// 
//  +------------+------+------------+
//  | hishf bits | uint | loshf bits |
//  ^------------+------+------------^
//  |                                |
// data                        (data + len) in byte

constexpr unsigned hishf_lead = (sizeof(unsigned) - 1) * 8;

template <unsigned len, unsigned hishf = 0, unsigned loshf = 0>
struct uint_view {
    byte_t* data;

    operator uint_t() const noexcept {
        if constexpr (std::endian::native == std::endian::big) {
            return parse_uint_be();
        }
        else {
            return parse_uint_le();
        }
    }

    void operator=(uint_t uint) const noexcept {
        if constexpr (std::endian::native == std::endian::big) {
            write_uint_be(uint);
        }
        else {
            write_uint_le(uint);
        }
    }

private:
    static_assert(2 <= len && len <= sizeof(uint_t));
    static_assert(hishf <= 7 && loshf <= 7);
    constexpr static unsigned hi_uint_mask = unsigned(-1) << hishf_lead << hishf >> hishf >> hishf_lead;
    constexpr static unsigned hi_data_mask = ~hi_uint_mask << hishf_lead >> hishf_lead;
    constexpr static unsigned lo_uint_mask = unsigned(-1) << hishf_lead >> hishf_lead >> loshf << loshf;
    constexpr static unsigned lo_data_mask = ~lo_uint_mask << hishf_lead >> hishf_lead;

    uint_t parse_uint_le() const noexcept {
        uint_t uint{};
        auto dst = (byte_t*)(&uint);
        auto src = data + (len - 1);
        do {
            *dst++ = *src--;
        }
        while (src > data);
        *dst = *src & hi_uint_mask;
        return uint >> loshf;
    }

    uint_t parse_uint_be() const noexcept {
        uint_t uint{};
        auto dst = (byte_t*)(&uint) + (sizeof(uint_t) - len);
        auto src = data;
        *dst++ = *src++ & hi_uint_mask;
        std::memcpy(dst, src, len - 1);
        return uint >> loshf;
    }

    void write_uint_le(uint_t uint) const noexcept {
        uint <<= loshf;
        auto dst = data + (len - 1);
        auto src = (byte_t const*)(&uint);
        *dst = (*dst & lo_data_mask) | (*src & lo_uint_mask);
        --dst;
        ++src;
        while (dst > data) {
            *dst-- = *src++;
        }
        *dst = (*dst & hi_data_mask) | (*src & hi_uint_mask);
    }

    void write_uint_be(uint_t uint) const noexcept {
        uint <<= loshf;
        auto dst = data;
        auto src = (byte_t const*)(&uint) + (sizeof(uint_t) - len);
        *dst = (*dst & hi_data_mask) | (*src & hi_uint_mask);
        std::memcpy(++dst, ++src, len - 2);
        dst += len - 2;
        src += len - 2;
        *dst = (*dst & lo_data_mask) | (*src & lo_uint_mask);
    }
};

template <unsigned hishf, unsigned loshf>
struct uint_view<1, hishf, loshf> {
    byte_t* data;

    operator uint_t() const noexcept {
        return (*data & uint_mask) >> loshf;
    }

    void operator=(uint_t uint) const noexcept {
        uint <<= loshf;
        *data = (*data & data_mask) | (uint & uint_mask);
    }

private:
    static_assert(hishf <= 7 && loshf <= 7);
    static_assert(hishf + loshf <= 7);
    constexpr static unsigned uint_mask = unsigned(-1) << hishf_lead << hishf >> hishf >> hishf_lead >> loshf << loshf;
    constexpr static unsigned data_mask = ~uint_mask << hishf_lead >> hishf_lead;
};

//     _ _ _ _ _ _ _ _
// pos 0 1 2 3 4 5 6 7

template <unsigned pos>
using bit_view = uint_view<1, pos, 7 - pos>;

template <unsigned len, unsigned hishf, unsigned loshf> inline
std::ostream& operator<<(std::ostream& os, uint_view<len, hishf, loshf> view) {
    return os << uint_t(view);
}

} // namespace net

template <unsigned len, unsigned hishf, unsigned loshf>
struct std::formatter<net::uint_view<len, hishf, loshf>>: formatter<net::uint_t> {
    auto format(net::uint_view<len, hishf, loshf> view, format_context& ctx) {
        return formatter<net::uint_t>::format(view, ctx);
    }
};
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Commented May 10, 2023 at 14:41

1 Answer 1

Reset to default
2
\$\begingroup\$

Handling corner cases

Pertaining to correctness, I know my code fails for cases like "4 bits + 7 bytes + 4 bits". It's a legit input for the payload fits into a 64-bit uint.

This is something that probably will not ever happen. Protocols are usually designed such that values naturally align as best as possible, so in a sane design the two 4 bit fields would be packed together in one byte, and also likely so the 7 byte field would only need to have a mask applied, and not a shift. That said:

I cannot come up with an algorithm that handles this without sacrificing too much performance. So I decided not to support it.

First of all, your class uint_view is templated, so you can just use if constexpr to keep the fast code fast, so there is no need to sacrifice any performance.

Second, it should not be a problem to support this. It's just a shift and a mask of 64-bit values. The issue you are having is that you are calculating the mask in the wrong way:

Calculating masks

The way you are calculating the masks is way too complicated. Instead of using unsigned(-1), use UINT64_MAX to get a 64-bit value with all one-bits, and shift that as necessary. You should also need just one mask:

constexpr std::uint64_t mask = (UINT64_MAX << (loshf + hishf)) >> hishf;

You just apply that mask to the whole 64-bit value, and then shift the value left or right as necessary. To get the inverse of the mask, just write ~mask.

Don't create aliases for standard types

Creating a type alias uint_t is not helpful. It sounds like "unsigned integer", so if I didn't know any better I would assume it's only 32 bits on most platforms. I now have to look at the definition to find out it's actually 64 bits. I recommend you use std::uint64_t everywhere instead; it's not that much more to type, and now there is no chance of any confusion.

The same goes for byte_t. Even better than using std::uint8_t though, there is a std::byte since C++17.

Avoid code duplication

You have two versions of the parse_*() and write_*() functions. I think it would be nicer to split the byte swapping from the shifting and masking. In particular, see if you can write those functions like so:

// Do things before swapping
uint64_t value;
…

// Do the swap
if constexpr (std::endian::native == std::endian::little) {
    value = std::byteswap(value);
}

// Do things needed after swapping
…

Here I'm using C++23's std::byteswap(), if you can't use that yet you can provide your own function that swaps bytes.

Const issues

Your write_*() member functions are marked const. While technically you are not modifying any member variable of uint_view itself, consider what happens if you parse a value, write to it, then parse it again. Can the compiler optimize out the second call to the parse function and assume it returns the same value is the first time it was called?

Your class also doesn't handle views of const network packets. While you could create a class const_uint_view, that would result in a lot of duplication. Another option would be to make it a template, like std::span, where the constness of the data viewed is captured by the template parameter.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks for taking time looking into the code. 1) Agree that the corner case seldom happens. So, don't bother for now. 2) I'm used to the some_uint_type(-1) idiom. Seen it in std lib implementation like std::string::npos. \$\endgroup\$ Commented May 10, 2023 at 13:11
  • \$\begingroup\$ 3) (UINT64_MAX << (loshf + hishf)) >> hishf looks incorrect. hishf and loshf refer to bits to most significant and least significant ends, respectively. I tend to use one-byte mask to the least significant end and zero out all other bytes (though may not be necessary). I did some simplification in mask derivation. Now, it's like hi_uint_mask = (unsigned(-1) << hishf & 0xff) >> hishf and hi_data_mask = ~hi_uint_mask & 0xff. \$\endgroup\$ Commented May 10, 2023 at 13:11
  • \$\begingroup\$ 4) I used uint_t in an attempt to gain flexibility and hide implementation detail. I expect user not to assume uint_t width which is subject to change. I used byte_t alias in an attempt to clarify its usage (not be used as integers). Yeah, I should consider switching to std::byte. \$\endgroup\$ Commented May 10, 2023 at 13:11
  • \$\begingroup\$ 5) I wrote separate "le" and "be" versions mainly in an attempt to improve performance. Your suggestion requires one additional pass to swap bytes in the worst case. 6) I did think about const correctness. The view classes are like pointers. So I try to implement the semantics that the const applies to the view itself, not the underlying data being viewed. Indeed, I can parameterize the type of data like T* data. So user can pass byte_t or byte_t const. \$\endgroup\$ Commented May 10, 2023 at 13:12
  • 1
    \$\begingroup\$ Ah I see you want to do it a byte at a time, and only mask the first and last byte. My approach would be to get everything into a uint64_t variable and do all shifting/masking/swapping there. As for performance, compilers are very good at optimizing this sort of thing, especially if they know everything at compile-time. Furthermore, many processors have instructions to byte-swap whole registers at a time. Best would be to check the assembly output of your compiler and/or to benchmark your code. \$\endgroup\$ Commented May 10, 2023 at 13:31

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.