Packing and unpacking values in a buffer

Question

I have to deal with raw memory manipulation. For that I wrote a function which stores data one after another, and another function which reads this data and stores it into variables.

More precisely:

/**
 * - `void * pack( dest, src1, ..., srcn )`
 *   `void * pack( void * dest, src1, ..., srcn )`
 *    Copies the bytes from values of src1, ..., srcn one after another to the address of dest
 *    The sum of sizeof(srci) must equal sizeof(dest), except, when dest is a void pointer in which no type checking is performed
 *    If an address of vari points to some memory of dest, the behaviour is undefined
 *    Returns a pointer one after the last written byte
 *
 * - `void unpack( src, dest1, ..., destn )`
 *   `void unpack( const void * src, dest1, ..., destn )`
 *    Similar to `pack`, but writes the content of src into the variables dest1, ..., destn
 *    Does not return anything
 *
 * ## Helpers
 *  - ´int64_t sum< ... >()`
 *    function  which returns the sum of all template arguments, arguments must be integers
 */

I want to know

• Do I invoke undefined behavior anyplace, anywhere, anytime? When so,
  • which tests I could write against it (i.e. which tests are likely to fail at the moment)?
  • what else could I do to get the correct pointer arithmetic here?
• Is the interface of the functions sensible?
• Is the __restrict__ qualifier of some use here?
• Any other comments are also appreciated.

#ifndef CSTRINGUTILS_HPP
#define CSTRINGUTILS_HPP

#include <cassert>
#include <cstring>
#include <cstdint>
#include <type_traits>



/// Macro making `restrict` keyword available
#ifndef restrict
    #if defined(__GNUC__) && ((__GNUC__ == 8) || (__GNUC__ == 9) || (__GNUC__ == 10))
        #define restrict __restrict__
    #else
        #warning "restrict keyword not defined via macro"
    #endif
#endif

namespace detail {

    inline void pack_worker( char * /*dest*/ ) noexcept { }
    template< typename SRC, typename ... TAIL >   inline void pack_worker( char * restrict dest, const SRC * const src, const TAIL * const ... tail ) noexcept {
        static_assert( std::is_trivially_copyable<SRC>::value, "pack cannot handle types which are not trivially copyable" );
        memcpy( dest, src, sizeof(SRC) );
        pack_worker( dest + sizeof(SRC), tail ... );
    }

    inline void unpack_worker( const char * /*src*/ ) noexcept { }
    template< typename DEST, typename ... TAIL >   inline void unpack_worker( const char * restrict src, DEST * dest, TAIL * ... tail ) noexcept {
        static_assert( std::is_trivially_copyable<DEST>::value, "unpack cannot handle types which are not trivially copyable" );
        memcpy( dest, src, sizeof(DEST) );
        unpack_worker( src + sizeof(DEST), tail ... );
    }

    template< int64_t ... > struct sum_worker : std::integral_constant< int64_t, 0 > { };
    template< int64_t X, int64_t ... Xs > struct sum_worker< X, Xs... > : std::integral_constant< int64_t, X + sum_worker<Xs...>::value > { };

}

template< int64_t ... Xs > inline constexpr int64_t sum() { return detail::sum_worker< Xs... >::value; };

template< typename DEST, typename SRC, typename ... TAIL >   inline void * pack( DEST & restrict dest, const SRC & src, const TAIL & ... tail ) noexcept {
    constexpr size_t sze = sum< sizeof(SRC), sizeof(TAIL)... >();
    static_assert( sizeof(DEST) == sze, "Sizes are not compatible. If this is intended, pass `&dest` as `void*`." );
    detail::pack_worker( reinterpret_cast<char *>(&dest), &src, &tail ... );
    return static_cast< void* >( reinterpret_cast<char *>(dest) + sze );
}

template< typename SRC, typename ... TAIL >   inline void * pack( void * restrict dest, const SRC & src, const TAIL & ... tail ) noexcept {
    constexpr size_t sze = sum< sizeof(SRC), sizeof(TAIL)... >();
    detail::pack_worker( reinterpret_cast<char *>(dest), &src, &tail ... );
    return static_cast< void* >( reinterpret_cast<char *>(dest) + sze );
}

template< typename SRC, typename DEST, typename ... TAIL >   inline void unpack( const SRC & restrict src, DEST & dest, TAIL & ... tail ) noexcept {
    constexpr size_t sze = sum< sizeof(DEST), sizeof(TAIL)... >();
    static_assert( sizeof(SRC) == sze, "Sizes are not compatible. If this is intended, pass `&src` as `const void*`." );
    detail::unpack_worker( reinterpret_cast<char const *>(&src), &dest, &tail ... );
}

template< typename DEST, typename ... TAIL >   inline void unpack( const void * restrict const src, DEST & dest, TAIL & ... tail ) noexcept {
    detail::unpack_worker( reinterpret_cast<char const *>(src), &dest, &tail ... );
}

#endif //CSTRINGUTILS_HPP

Here is my set of unit tests. (In my own project, I use googletest, and thus, the tests can be written more concisely.)

int main() {

    // GeneralTest
    {
        int8_t c1 = 0xAA, c2 = 0xBB, c1r, c2r;
        int16_t dest;
        pack( dest, c1, c2 );

        int16_t res = 0xBBAA;
        assert( memcmp(&dest, &res, sizeof(dest)) == 0 );

        unpack( dest, c1r, c2r );
        assert( memcmp(&c1, &c1r, sizeof(c1)) == 0 );
        assert( memcmp(&c2, &c2r, sizeof(c2)) == 0 );
        assert( memcmp(&dest, &res, sizeof(dest)) == 0 );
    }
    {
        int16_t c1 = 0xAABB, c1r;
        int32_t c2 = 0xCCDD'EEFF, c2r;
        int8_t c3 = 0x11, c3r;
        int8_t c4 = 0x22, c4r;
        int64_t tmp;
        pack( tmp, c1, c2, c3, c4 );

        int64_t res = 0x2211'CCDD'EEFF'AABB;
        assert( memcmp(&tmp, &res, sizeof(tmp)) == 0 );

        unpack( tmp, c1r, c2r, c3r, c4r );
        assert( memcmp(&c1, &c1r, sizeof(c1)) == 0 );
        assert( memcmp(&c2, &c2r, sizeof(c2)) == 0 );
        assert( memcmp(&c3, &c3r, sizeof(c3)) == 0 );
        assert( memcmp(&c4, &c4r, sizeof(c4)) == 0 );
        assert( memcmp(&tmp, &res, sizeof(tmp)) == 0 );
    }


    // TemporaryTest
    {
        int32_t tmp;
        bool t[4];

        pack( tmp, true, true, false, true );
        unpack( tmp, t[3], t[1], t[2], t[0] );  // strange ordering is on purpose
        bool tr = true;
        bool fa = false;
        assert( memcmp(&t[3], &tr, 1) == 0 );
        assert( memcmp(&t[1], &tr, 1) == 0 );
        assert( memcmp(&t[0], &tr, 1) == 0 );
        assert( memcmp(&t[2], &fa, 1) == 0 );
    }
    {
        uint8_t t[8];
        unpack( 0xAABBCCDD, t[0], t[2], t[4], t[6] );  // strange ordering is on purpose
        uint8_t r0 = 0xDD, r2 = 0xCC, r4 = 0xBB, r6 = 0xAA;
        assert( memcmp(&t[0], &r0, 1) == 0 );
        assert( memcmp(&t[2], &r2, 1) == 0 );
        assert( memcmp(&t[4], &r4, 1) == 0 );
        assert( memcmp(&t[6], &r6, 1) == 0 );
    }


    // VoidTest
    {
        char tmp[4];
        pack( (void*)(&tmp), true, false, false );
        bool tr = true;
        bool fa = false;
        assert( memcmp(&tmp[0], &tr, 1) == 0 );
        assert( memcmp(&tmp[1], &fa, 1) == 0 );
        assert( memcmp(&tmp[1], &fa, 1) == 0 );

        bool b1, b2, b3;
        unpack( (const void*)(&tmp), b1, b2, b3 );
        assert( memcmp(&b1, &tr, 1) == 0 );
        assert( memcmp(&b2, &fa, 1) == 0 );
        assert( memcmp(&b3, &fa, 1) == 0 );
    }



    // PackTestSizeTest
    {
        // int8_t c1 = 0xAA, c2 = 0xBB, c3 = 0xCC;
        // int16_t tmp;
        // pack( tmp, c1, c2, c3 );  // Must not compile
        // pack( tmp, c1 );  // Must not compile
    }
    {
        // int tmp;
        // pack( tmp, 10, 20 );  // Must not compile
    }

}

Answer 1

1. Your interface is needlessly inconsistent.

   Just let both packing and unpacking return a pointer just past the used buffer.

2. sum() has the wrong semantics for your use-case. You really want std::size_t, and saturating math. Also, if you use a function, use function-arguments not template-arguments.

   sum_sat() might be a good name.

3. Consider adding an additional API, checking whether the buffer contains the packed objects.

4. The functions accepting the buffer by reference and by void-pointer are nearly identical.

   Just delegate to the unchecked void-pointer version after performing the check.

5. If you can, upgrade so you can use C++17 fold-expressions and C++20 concepts + abbreviated templates.

   They just make it easier to write and better to use. I'll stay at C++14 for now.

6. Don't include a header in your header just because your implementation (or tests) need it. Put it where it belongs and isn't pulled in for everyone.

7. <cstring> and <cstdint> might also put their symbols in the global namespace. Don't depend on it, they will put them into std for sure.

8. Correct code stays correct code if all use of restrict is removed. Thus, fall back to defining it to nothing.

9. Keep your lines short. Long lines kill readability, especially if they cause horizontal scrolling or line-wrapping by the editor.

#ifndef CSTRINGUTILS_HPP
#define CSTRINGUTILS_HPP

#include <cstring>
#include <type_traits>
#include <limits>

/// Macro making `restrict` keyword available
#ifndef restrict
    #if defined(__GNUC__) && ((__GNUC__ == 8) || (__GNUC__ == 9) || (__GNUC__ == 10))
        #define restrict __restrict__
    #else
        #define restrict
        #warning "restrict has no effect, leading to inefficient code"
    #endif
#endif

constexpr auto sum_sat() noexcept { return '\0'; }
template <class T, class... Us>
constexpr auto sum_sat(T a, Us... us) noexcept {
    constexpr auto b = sum_sat(us...);
    constexpr auto max = std::numeric_limits<std::decay_t<decltype(a + b)>>::max();
    return max - b >= a ? max : a + b;
}

#define X(name, op, c1, c2) \
    inline c1 void* name(c1 void* restrict p) noexcept { return p; } \
    \
    template <class T, class... Us> \
    inline c1 void* name(c1 void* restrict p, c2 T& t, c2 Us&... us) noexcept { \
        static_assert(std::is_trivially_copyable<T>::value, \
            "Objects must be trivially copyable."); \
        op; \
        return name((c1 void*)((c1 char*)p + sizeof t), Us...); \
    } \
    \
    template <class T, class... Us> \
    inline c1 void* name(c1 T& restrict t, c2 Us&... us) noexcept { \
        static_assert(sizeof t == sum_sat(sizeof Us...), "Buffer has wrong size."); \
        static_assert(std::is_trivially_copyable<T>::value, \
            "Buffer has to be trivially copyable."); \
        return name((c1 void*)&t, us...); \
    }
X(pack, std::memcpy(p, &t, sizeof t),,const)
X(unpack, std::memcpy(&t, p, sizeof t),const,)
#undef X

#endif //CSTRINGUTILS_HPP

The above code was only proved correct, not compiled or run.

Using an X-macro might be a bit OTT, but OP seemed enthusiastic.

Answer 2

Auto-review by compiler:

g++-10 -std=c++20 -fconcepts -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds  -Weffc++       262935.cpp    -o 262935
262935.cpp:41:105: warning: extra ‘;’ [-Wpedantic]
   41 | template< int64_t ... Xs > inline constexpr int64_t sum() { return detail::sum_worker< Xs... >::value; };
      |                                                                                                         ^
262935.cpp: In function ‘int main()’:
262935.cpp:74:21: warning: overflow in conversion from ‘int’ to ‘int8_t’ {aka ‘signed char’} changes value from ‘170’ to ‘-86’ [-Woverflow]
   74 |         int8_t c1 = 0xAA, c2 = 0xBB, c1r, c2r;
      |                     ^~~~
262935.cpp:74:32: warning: overflow in conversion from ‘int’ to ‘int8_t’ {aka ‘signed char’} changes value from ‘187’ to ‘-69’ [-Woverflow]
   74 |         int8_t c1 = 0xAA, c2 = 0xBB, c1r, c2r;
      |                                ^~~~
262935.cpp:78:23: warning: overflow in conversion from ‘int’ to ‘int16_t’ {aka ‘short int’} changes value from ‘48042’ to ‘-17494’ [-Woverflow]
   78 |         int16_t res = 0xBBAA;
      |                       ^~~~~~
262935.cpp:87:22: warning: overflow in conversion from ‘int’ to ‘int16_t’ {aka ‘short int’} changes value from ‘43707’ to ‘-21829’ [-Woverflow]
   87 |         int16_t c1 = 0xAABB, c1r;
      |                      ^~~~~~

---

std::memcpy is misspelt throughout, as are all the types declared by <cstdint>.

---

Naming: all-caps is conventionally reserved for preprocessor macros (which need to shout, as they are text substitutions that don't obey C++ rules such as scope). A common convention for template parameters is PascalCase.

---

There's no need to cast pointer types to void* - that's always a valid implicit conversion.

---

The restrict qualifier doesn't seem to be giving us any benefit other than documenting requirements to the user. It's not helping the compiler create better code, so we could use #define restrict /*restrict*/ or simply change it to documentation.

---

---

Use the features of modern C++

This section was written before the question was changed to target C++14. As such, it no longer applies here, but may be instructive for other readers.

---

In modern C++, I'd be more likely to use a requires clause than static_assert to disable inapplicable overloads. That said, you do have extra information ("If this is intended, pass &dest as void*") which may be worth retaining.

---

We can reduce the number of templates by using fold expressions. For example, sum() and sum_worker reduce to just

    constexpr size_t sze = sizeof src + (sizeof tail + ...);

Using fold-expressions for the body of the code, it becomes much smaller and easier to reason about:

namespace detail {
    template<typename T>
    concept trivially_copyable = std::is_trivially_copyable_v<T>;
}

void pack(auto& dest, const detail::trivially_copyable auto& ...src) noexcept
    requires (sizeof dest == (sizeof src + ...))
{
    auto p = reinterpret_cast<char*>(&dest);
    ((std::memcpy(p, &src, sizeof src), p += sizeof src),...);
}

void *pack(void *dest, const detail::trivially_copyable auto& ...src) noexcept
{
    auto p = reinterpret_cast<char*>(dest);
    ((std::memcpy(p, &src, sizeof src), p += sizeof src),...);
    return p;
}

void unpack(const auto& src, detail::trivially_copyable auto& ...dest) noexcept
    requires (sizeof src == (sizeof dest + ...))
{
    auto p = reinterpret_cast<const char*>(&src);
    ((std::memcpy(&dest, p, sizeof dest), p += sizeof dest),...);
}

const void *unpack(const void* src, detail::trivially_copyable auto& ...dest) noexcept
{
    auto p = reinterpret_cast<const char*>(src);
    ((std::memcpy(&dest, p, sizeof dest), p += sizeof dest),...);
    return p;
}

That gets a clean Valgrind run, on the same test suite.

---

Talking of test suite, this test is disabled:

       // pack( tmp, c1, c2, c3 );  // Must not compile

We can use the detection idiom to ensure that this will not compile.

My platform doesn't yet have std::is_detected, but it's easy to knock up a rough equivalent:

namespace {
    template <class AlwaysVoid, template<class...> class Op, class... Args>
    struct detector : std::false_type {};

    template <template<class...> class Op, class... Args>
    struct detector<std::void_t<Op<Args...>>, Op, Args...> : std::true_type {};

    template <template<class...> class Op, class... Args>
    constexpr bool is_detected_v = detector<void, Op, Args...>::value;
}

Then we write the tests using a simple template:

template<typename... Args>
using pack_exists = decltype(pack(std::declval<Args>()...));

static_assert(is_detected_v<pack_exists, std::uint16_t&, std::uint8_t, std::uint8_t>, "16 <= 8 + 8");
static_assert(!is_detected_v<pack_exists, std::uint8_t&, std::uint8_t, std::uint8_t>, "8 <= 8 + 8");
static_assert(!is_detected_v<pack_exists, std::uint16_t&, std::uint8_t>, "16 <= 8");

We should add detection tests for unpack().

We don't yet have any tests of the versions which return pointers; they should be added.

Answer 3

To add to the previous answer, you don't have to say inline for template functions. They always have the property of merging duplicates during link-time and always work in headers. (It's not a hint to inline the function anymore... the compiler does that by itself. inline means this definition can appear in multiple translation units and should all refer to the same thing.)

It's orthodox in C++, ever since Strustrup's first book, to put the * modifier with the type not with the declarator. So write void* p; not void *p;.

Publishing the restrict macro in the header file will contaminate any files that includes it. What if the user didn't want that, or even uses the word as a variable name somewhere?

Answer 4

Pass the size of the buffer

Your overloads that take a void * to a buffer will happily read or write past the end of that buffer, since they don't know how long the buffer is. Consider letting the pack and unpack functions know the size of the buffer some way, either by passing a std::size_t or by passing a pointer to the end of the buffer. The latter has the advantage that it doesn't have to be updated if you want to call pack() or unpack() multiple times for the same buffer. Also consider how you want to let the caller know if the buffer would have been overrun somehow.

If the user really wants to use void pointers, she should be aware of the dangers.

Sure, but it helps if we have something in place to let us know when we do make a mistake. It's easy to implement:

void *pack(void *dest, std::size_t len, const auto &... src) noexcept
{
    assert(len == (sizeof src + ...));
    ...
}

If the given size is correct and it is a constant, the assertion will be optimized away.