6
\$\begingroup\$

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
    }

}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ I think it's reasonable to include your tests using Google Test, even if you also provide the main() for convenience. That allows us to review the tests properly, too (e.g. naming, layout, use of EXPECT_TRUE instead of EXPECT_EQ). I routinely post questions including Google Test cases; nobody has complained yet! \$\endgroup\$ Jun 11, 2021 at 14:47

4 Answers 4

2
\$\begingroup\$
  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.

\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ He's not using concepts/requires, nor the handy _v forms of type traits. So I suspect he's maintaining compatibility with older compilers. It clearly supports gcc 8, which doesn't have C++20 concepts (you can enable the Concepts TS, which has one syntax difference, but there's no standard library definitions) \$\endgroup\$
    – JDługosz
    Jun 11, 2021 at 17:26
  • \$\begingroup\$ I had skipped straight past the GCC 8 support, which does suggest an older version of C++. But since it's not tagged for a specific language version, "Use C++'s newer features" is a valid answer, and I think it's good to showcase the benefits of the newer standard (it's not just specification bloat - it does actually help us developers!). \$\endgroup\$ Jun 11, 2021 at 19:32
  • \$\begingroup\$ I forgot to tag it with C++14. I need to use this stuff on CUDA as well, and thus, cannot use more modern stuff at the moment. Could you also say something about my concern, whether this code has UB? I am not 100% sure whether my casts and the pointer arithmetics are justified. \$\endgroup\$
    – tommsch
    Jun 11, 2021 at 20:24
  • \$\begingroup\$ you forgot to restrict the buffer-reference. And your return-values are inconsistent. \$\endgroup\$ Jun 12, 2021 at 20:33
  • 1
    \$\begingroup\$ Unfortunately I can accept only one ansers. \$\endgroup\$
    – tommsch
    Jun 17, 2021 at 11:33
2
\$\begingroup\$

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?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Fixing the restrict problem the obvious way (with unqualified #undef restrict at the end of the header) has the opposite problem: a user may have defined restrict for their own purposes. I think the simplest fix is just to remove the definition, as it doesn't bring any benefit even when __restrict__ is available, as far as I can tell. \$\endgroup\$ Jun 11, 2021 at 19:39
  • 1
    \$\begingroup\$ NB inline: I found on godbolt, that gcc feels more inclined to inline templates when the inline keyword is there. I didnt check it with this example, but since then I just add it. NB restrict: good point, but I do not plan to publish this code NB pointers: I know, but since Cs pointer syntax is really weired (pointer to function/function returning pointers) I settled to put the star alone. \$\endgroup\$
    – tommsch
    Jun 11, 2021 at 20:35
  • 1
    \$\begingroup\$ I know this is bikeshedding, but void *p; is used by some default clang-format styles, like LLVM. So, it's not quite unorthodox to put * with the variable name. \$\endgroup\$ Jun 12, 2021 at 11:23
  • \$\begingroup\$ Don't use such a format style with C++. It will confuse and annoy those who would read the code. You could put ; at the beginning of a line rather than the end, or you could right-justify your code, but just don't. Stroustrup introduced the void* p formatting in his original book on C++ in the 80's along with an explanation of the reasoning behind it. Look at the C++ standard document itself, in the samples and in the signatures of the library functions. Adopting a contrary style because it's available in the box (autoformat) is not a good reason. \$\endgroup\$
    – JDługosz
    Jun 14, 2021 at 13:49
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ The reference versions of the two functions do check the buffer size. Only the void pointer versions do not check it. If the user really wants to use void pointers, she should be aware of the dangers. \$\endgroup\$
    – tommsch
    Jun 13, 2021 at 19:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.