8
\$\begingroup\$

I've written some simple helper functions that read data types from a vector of bytes (binary files) and having a tough time deciding which route to go.

Method 1 : reinterpret_cast

int32_t read_32s(const std::vector<uint8_t> &buf, const unsigned offset,
                 const bool bswap = false) {
  // Check for out of bounds
  if (offset > buf.size() - sizeof(int32_t)) {
    // error handling
  }
  // Cast the pointer to int32_t and get the first value
  const auto val = reinterpret_cast<const int32_t *>(&buf[offset])[0];
  // Swap bytes if necessary
  if (bswap) {
    return (((val & 0xFF000000) >> 24) | ((val & 0x00FF0000) >> 8) |
            ((val & 0x0000FF00) << 8) | ((val & 0x000000FF) << 24));
  }
  return val;
}

The biggest advantage with this method is probably performance, readability is debatable. This basically compiles down to a single instruction, two if you need to swap the bytes. The problem I have with this is casting like this in modern C++ is frowned upon, and I am unsure how safe it actually is despite bounds checking first. How does alignment work?

Method 2 : classic bit shifts

int32_t read_32s(const std::vector<uint8_t> &buf, const unsigned offset,
                 const bool bswap = false) {
  // Check for out of bounds
  if (offset > buf.size() - sizeof(int32_t)) {
    // error handling
  }
  // Swap bytes if necessary
  if (bswap) {
    return (buf[offset] << 24) | (buf[offset + 1] << 16) |
           (buf[offset + 2] << 8) | buf[offset + 3];
  }
  return (buf[offset + 3] << 24) | (buf[offset + 2] << 16) |
         (buf[offset + 1] << 8) | buf[offset];
}

Very simple bit shifting, about double the instructions as the first method, and can get a bit messy with larger data types. I presume this is the safer solution though, and perhaps more readable.

Is the first method OK, or would you consider it to be poor code in comparison to the second method? Or maybe there's just a better method all together (std::byte?).

Here is a compiler explorer link with both examples.

\$\endgroup\$

closed as off-topic by Snowhawk, Stephen Rauch, Mast, hjpotter92, Graipher Jul 22 '18 at 18:00

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – Snowhawk, Stephen Rauch, Mast, hjpotter92, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ I wonder why do you need bswap at all. Is it to swap bytes from a known endianness to host? If that's the case...You should never ever do it (and you may want to double check your second solution) \$\endgroup\$ – Adriano Repetti Jul 21 '18 at 10:19
  • 1
    \$\begingroup\$ Which one is best might well depend on in which context it's used. Context which is currently missing. Please take a look at the help center. \$\endgroup\$ – Mast Jul 21 '18 at 11:27
  • \$\begingroup\$ The swap is in the case of binary files where the byte order may be either way. TIFFs for example can be either way, and the header describes the endianess. \$\endgroup\$ – ricco19 Jul 21 '18 at 13:29
  • \$\begingroup\$ Yep, that's a pretty good reason! 🙌 \$\endgroup\$ – Adriano Repetti Jul 21 '18 at 15:09
8
\$\begingroup\$

You are correct to be concerned about the first method. It generates UB if buf[offset] doesn't happen to be at the right alignment boundary for a 32-bit value.

The second method is the way to go, even if you don't care about portability (but as a bonus, it's portable, except for the use of int32_t).

As mentioned by @AJNeufeld you need to fix up your out-of-bound checks. Once that's done, you could simplify the function quite a bit by using std::swap(). For example:

if (bswap) {
    swap(buf[offset    ], buf[offset + 3]);
    swap(buf[offset + 1], buf[offset + 2]);
}
return (buf[offset + 3] << 24) | (buf[offset + 2] << 16) |
    (buf[offset + 1] << 8) | buf[offset];

That's even pretty easy to generalize to any size with a couple of loops (that will almost certainly be unrolled).

EDIT: As @hoffmale notes in the comments, the return line is still possibly UB. The reason it technical.

auto b = int8_t{};
auto r = b << 8;

The above code is legal because in every arithmetic operation, signed chars and shorts are always promoted to int. So b is promoted to int, which must be at least 16 bits, and the shift is cool, and r is int.

The problems arise when you shift by more than 8 bits. The types will always be promoted to int, but int is only guaranteed to be 16 bits. That means if you did:

auto r = b << 16; // or 24

now you have a problem.

To solve it, you need to make sure the value being shifted is at least 32 bits before the shift:

auto r = uint_fast32_t{b} << 16; // or 24

This works portably.

So you'd need to add this cast before each shift in the return line.

But a better way to handle this - one that scales - is to use a loop:

auto ret = int32_t{};
for (auto i = 0; i < 4; ++i)
{
    ret << 8;
    ret |= buf[offset + (3 - i)];
}

or even:

auto ret = int32_t{};
for (auto i = 0; i < 4; ++i)
{
    ret << 8;
    if (bswap)
        ret |= buf[offset + i];
    else
        ret |= buf[offset + (3 - i)];
}

and now you don't have to worry about promotions. Any decent optimizing compiler will hoist the invariant conditional and offset out of the loop and unroll the loop (and eliminate the unnecessary shift). And you can even template the loop on the size and reuse it.

Shifting and OR-ing is the only portable way to get the little-endian data in buf into an integer for all machines.

@juhist describes another option: using memcpy(). That's not a portable solution for a number of technical reasons. First, you'll get different behaviour depending on the endianness of the machine. More technically, the machine may not always store integers the way you expect, and I'm not referring to endianness by that. You're dealing with signed values, but the type of signedness of the data may not be the same as the machine (for example, the data may be twos-complement, the machine may be signed magnitude). There may be padding bits and other weirdness going on that ends up meaning that a 32 bit value of 0xFFFFFFFF may not necessarily be represented in memory as 4 bytes of 0xFF. Or if your input data is "0x00 0x00 0x00 0xA0" which if memcpy()ed on a little endian machine will give you 0xA0000000 (that's its value, not its memory representation), which you assume is −2147483648... but that's assuming bit 31 is the sign bit, which is not necessarily true. Could be bit 0, for example, meaning this value is actually 1342177280. (Granted, if your data has a sign bit, you really need to handle it specifically in any case. That is, you'd need to calculate the high byte as something like buf[offset + 3] & 0x7Fu, and multiply the result by −1 if buf[offset + 3] & 0x80u... or you could dump the whole thing into an unsigned value, and then deal with the sign bit. But the point about the sign bit not being where you expect it still stands: 0x01 0x00 0x00 0x00, if memcpy()ed on a little-endian machine, could produce −2147483648 rather than 1... or anything else really.)

If performance really matters - and, I'll be blunt, if you're using this part of a serialization routine, it almost certainly doesn't; whatever time you spend decoding integers is going to be dwarfed by the I/O - you could write system-specific code. For example, if your system is little-endian, and you know the representation is the same as what's in the buffer, you could just memcpy(). If it's big-endian, maybe memcpy() and a swap intrinsic, or a loop that's basically reverse memcpy() (create the integer, then copy the bytes in reverse order). If it's other-endian or non-twos-complement or has some other quirck... well, you'd know how to deal with that in that case.

The bottom line is that shifting and OR-ing is the only portable solution (and you'd need to handle the sign bit properly). If you don't care about portability and you want speed, well, then the "right" answer depends entirely on 1) the data representation, and 2) your machine.

\$\endgroup\$
  • \$\begingroup\$ Thanks, the eventual goal is to generalize it for sure. Though I am curious how std::swap simplifies anything here as I am passing the buffer by const reference. Shifting the opposite way seems just as simple to me, and it seems silly to copy the bytes just to swap them. \$\endgroup\$ – ricco19 Jul 21 '18 at 2:13
  • \$\begingroup\$ Yes, if the buffer is const, swap won't help. Honestly, what I'd do if - like you say - you don't care about portability is just copy the buffer bytes into a 32-bit variable like you do in the return line, then conditionally use an intrinsic like __builtin_bswap32. With the function inlined and the instructions interleaved on a modern multi-piped core, the whole function's cost (not counting any error handling) would be below the noise threshold. \$\endgroup\$ – indi Jul 21 '18 at 3:38
  • \$\begingroup\$ This solution still contains undefined behavior: buf[offset + 3] << 24 might overflow (assuming 32-bit int), which is UB for signed integers before C++14. (Even with C++14, it's still undefined for platforms where int has less than 32 bits.) \$\endgroup\$ – hoffmale Jul 21 '18 at 10:49
  • \$\begingroup\$ @hoffmale That is true. So the correct thing to do would probably be to cast each buf[x] to uint32_t before the shift. \$\endgroup\$ – indi Jul 21 '18 at 11:17
6
\$\begingroup\$

I'm quite fine with the first approach. You are "reinterpreting" 4 bytes as a single 32-bit value.

However, I might want to replace the [0] array look-up with a straight dereference.

const auto val = * reinterpret_cast<const int32_t *>(&buf[offset]);

The first and second methods actually can do slightly different things.

The first method reads a 32 bit value using the computer's natural byte ordering, and then swaps the bytes if bswap is true.

The second method reads a 32 bit value in little endian format (if bswap is false) or big endian format (if bswap is true).

On a little endian machine, they are equivalent. On a big endian machine, they perform opposite.


I'm a little concerned with this code:

if (offset > buf.size() - sizeof(int32_t)) {
    // error handling
}

If the buffer has less than 4 bytes, the unsigned subtraction will overflow. The buffer size less the size of an int32 is not really a meaningful, physical quantity; it doesn't represent a concrete concept. What is important is that the starting point of the read plus the size of the value cannot extend beyond the end of the buffer. This can be represented in code as follows, and doesn't suffer from the unsigned subtraction overflow:

if (offset + sizeof(int32_t) > buf.size()) {
    // error handling
}
\$\endgroup\$
  • \$\begingroup\$ When I load the file I actually have a min and max set (> 8 < 10mb), but I should still be doing that anyways. And yeah, the [0] seemed a bit wonky. Thanks. \$\endgroup\$ – ricco19 Jul 20 '18 at 19:16
  • \$\begingroup\$ Also, I'm not concerned with system endianess or portability here. \$\endgroup\$ – ricco19 Jul 20 '18 at 19:20
  • \$\begingroup\$ -1. The first approach causes undefined behavior. You should neither be fine with it, nor endorse it without pointing that out first and explaining how to a) avoid it or b) make the compiler guarantee reasonable behavior in that particular instance. \$\endgroup\$ – Ben Steffan Jul 21 '18 at 1:00
4
\$\begingroup\$

Not only does your first version, as mentioned in indi's answer, cause undefined behavior on unaligned access, but the behavior is in fact undefined in any case, since you are violating aliasing rules. Unless you make sure that your compiler turns the other cheek (gcc, for example, does so with -fno-strict-aliasing), don't use that variant, ever.

Even if your compiler does not implement any optimizations based on that case of undefined behavior, your program could still crash: Assuming that unaligned accesses work on x86 is a common misbelief, and has been the cause of quite a few bugs since compilers have added SIMD code generation to their backends. That being said, it's not improbable that your compiler will recognize the undefined behavior and optimize based on it, either. I wouldn't be surprised if you ended up with a function that returned 0 in all cases, or that returns some uninitialized value, or similar.

\$\endgroup\$
  • \$\begingroup\$ Indeed, there were no compiler warnings with pretty pedantic settings, but the sanitizers complained about alignment a few times during runtime. UB probably not worth the couple nanoseconds! Thanka \$\endgroup\$ – ricco19 Jul 21 '18 at 2:19
2
\$\begingroup\$

None of the proposed approaches in the question or in the answer are correct and fast at the same time.

If you want a portable high-performance way to access a byte array as 32-bit integers, you're looking for memcpy:

static inline uint32_t get_int32(const void *buf)
{
  uint32_t res;
  memcpy(&res, buf, sizeof(res));
  return res;
}

An optimizing compiler will notice this is a 32-bit load on a possibly unaligned location, and will generate code accordingly. At least on i386 and AMD64 architectures the generated code is extremely fast.

Any form of reinterpret_cast will be undefined behavior due to strict aliasing rules and possibly due to alignment constraints. memcpy accesses the array as a character array, and any type of array can be accessed as character array using the aliasing rules. Also, memcpy is not alignment-dependent.

Any form of bit-shifting and bitwise OR, although correct, is terribly slow.

If you want to do byte swap from little endian to big endian on a big endian machine, you should really take a hard look at __builtin_bswap32, supported on at least GCC. Your compiler may be different, so use #ifdef and #if compiler preprocessor directives to identify the compiler and use its builtin. Most compilers have some fast way to perform high-performance 32-bit byte swap.

However, usually data is in big-endian and you want to access it in host byte order whether it's little- or big-endian. Then htonl and ntohl help you. Note htonl and ntohl are platform-dependent, but most platforms provide them.

\$\endgroup\$
  • 1
    \$\begingroup\$ You should probably mention that the memcpy() solution, while not UB, is IB: it will give you different answers on different systems. \$\endgroup\$ – indi Jul 21 '18 at 12:21
  • \$\begingroup\$ @indi I'm not 100% certain about that... If you originally serialized the data on the same machine (using memcpy to store it, of course), it works just fine. About the only implementation-defined thing is the byte order and for that we have htonl and ntohl. \$\endgroup\$ – juhist Jul 21 '18 at 12:23
  • \$\begingroup\$ You're making assumptions about the data that don't really match what's in the original code (method 2, because method 1 is broken). In the original code, the data is serialized little endian, and bswap makes it big-endian. If the data is 0x01 0x00 0x00 0x00, you get 1 on all platforms when bswap is false, and 16777216 on all platforms when it's true. memcpy() will give you different results on different platforms. That's worth noting, at least. \$\endgroup\$ – indi Jul 21 '18 at 12:28
  • \$\begingroup\$ I like this solution a lot actually. As for the builtin bswap, I believe if you write out the shifts on an int manually it gets optimized to a single bswap instruction anyways, which I think I prefer over preprocessor stuff. \$\endgroup\$ – ricco19 Jul 21 '18 at 13:56

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