Is is okay to use undefined behavior if it works?

[zbjornson]

Hello! In the code below, I'm accepting some buffer (1-byte aligned) and swapping bytes as if it's an array of int16s. (Full code has equivalent methods for 32 and 64 bit types.)

#if defined(__GNUC__) || defined(__clang__)
#define BSWAP_INTRINSIC_2(x) __builtin_bswap16(x)
#elif defined(__linux__)
#include <byteswap.h>
#define BSWAP_INTRINSIC_2(x) bswap_16(x)
#elif defined(_MSC_VER)
#include <intrin.h>
#define BSWAP_INTRINSIC_2(x) _byteswap_ushort(x);
#else
#define BSWAP_INTRINSIC_2(x) (x << 8) | (x >> 8)
#endif

char* data; // some char buffer

// For debugging:
int alignment = reinterpret_cast<uintptr_t>(data) % alignof(uint16_t);
std::cout << "aligned? " << alignment << "\n";

uint16_t* data16 = reinterpret_cast<uint16_t*>(data);

for (size_t i = 0; i < length_of_data16; i++) {
  data16[i] = BSWAP_INTRINSIC_2(data16[i]);
}

Sometimes, the `data16` pointer is not 2-byte aligned. Nonetheless, all of the possible definitions of BSWAP_INTRINSIC_2 work (i.e. MSVC, g++, clang; byteswap.h's bswap_16; and the bit-shifting fall through). In the assembly I see the compilers emitting MOVDQU, so they clearly identify that this might not be aligned and they're handling it. I also don't see instructions that are specific to 16-bit types.

1. As I understand, it's technically undefined behavior to cast a char* to a uint16_t* without verifying alignment. Is that true, or would it only be undefined behavior to do something based on the pointer being a 16-bit type after the cast?

2. Because this code works, how bad is it to use it?

3. Is there a way to make this legal without using memcpy/memmove/etc. (memory impact is too high for big buffers) and while still making use of the fast SSE instructions that are currently being used (e.g. PSHUFB)?


Thanks!

[coder777]

There is no undefined behavior. The pointer is not 16 bit after cast. The only thing is that the data length might be out of bounds.

The BSWAP_INTRINSIC_2() [probably] corrects the byte oder of the numeric value.

[JLBorges]

Something like this, perhaps:

template < typename T > bool is_aligned( void* ptr, std::size_t n ) {

    void* p = ptr ;
    return std::align( alignof(T), sizeof(T), p, n ) == ptr ;
}

void do_swap_bytes_intrinsic( char* buffer, std::size_t n ) {

    std::uint16_t* data16 = reinterpret_cast<std::uint16_t*>(buffer);
    for( std::size_t i = 0 ; i < n/2 ; ++i ) data16[i] = BSWAP_INTRINSIC_2( data16[i] );
}

void do_swap_bytes( char* buffer, std::size_t n ) {

        for( std::size_t i = 0 ; i < n ; i += 2 ) std::swap( buffer[i], buffer[i+1] ) ;
}

void swap_bytes( char* buffer, std::size_t n ) {

    if( is_aligned<std::uint16_t>( buffer, n ) ) do_swap_bytes_intrinsic( buffer, n ) ;
    else do_swap_bytes( buffer, n ) ;
}

http://coliru.stacked-crooked.com/a/f10d879047151057

[newbieg]

To answer the title question;
The problem with undefined behavior is that it is undefined, it does not follow a standardised action. So if you write an elaborate program with only one or two occurences of undef's, what is your guarantee that the program will do what you expect when the source is compiled on a different computer? What if that other computer uses a different compiler? What if the undefind behavior causes access to garbage information that is changed every time you restart your computer?
The reason to avoid undefined behavior is because they are not 100% predictable.

[Duthomhas]

To continue the answer to the title question:

No, it is never okay.

Even on the very same compiler you can change something completely unrelated to the statement in question and get different results. Anything can go wrong -- which is why it is called "undefined behavior" -- the compiler is not required to behave in any specific way, or to even consider the possibility that your code might be meaningful in any way.

In a sense, it's like feeding random objects to a chipper.
Chippers are designed to process trees.
If you feed them a steel girder, the chipper is perfectly within its design limits to fail -- even if sometimes it does not.

Hope this helps.

[zbjornson]

Thanks all.

@coder777 Could you elaborate a bit please? I was a bit sloppy and didn't mean that the pointer is 16-bits, but that the type of data it points to is 16-bits. (As you pointed out, I omitted from my example the length % 2 == 0 check.) If there's any way to consider this *not* UB, I'll be thrilled.

[coder777]

I don't know what to elaborate?

Your code snipped does not contain any undefined behavior.

If you get data from a byte oriented device such as a socket/file/etc. you need to reinterpret those data at some point. That is common practice.

What I cannot say is how safe it is to assume that the data is an uint16_t array.

[JLBorges]

Obvious undefined behaviour if alignof(std::uint16_t) > 1, and the data is not aligned.

Compiled with -fsanitize=undefined

#include <cstdint>
#include <iostream>

// compile with -fsanitize=undefined
static_assert( alignof(std::uint16_t) > alignof(char), "" ) ;
static_assert( alignof(std::uint32_t) >= 4, "" ) ;
using pvoid = const void* ;

void foo( char* buffer ) { // pass buffer not aligned for std::uint16_t

    std::cout << "*** foo called with buffer at: " << pvoid(buffer) << '\n' << std::flush ;
    std::uint16_t* data16 = reinterpret_cast<std::uint16_t*>(buffer);
    data16[0] =  __builtin_bswap16( data16[0] ) ; // *** runtime error: load of misaligned address
                                                  // *** runtime error: store to misaligned address
}

void bar( char* buffer ) { // pass buffer not aligned for std::uint32_t

    std::cout << "*** bar called with buffer at: " << pvoid(buffer) << '\n' << std::flush ;
    std::uint32_t* data32 = reinterpret_cast<std::uint32_t*>(buffer);  
    *data32 =  __builtin_bswap32( *data32 ) ; // *** runtime error: load of misaligned address
                                              // *** runtime error: store to misaligned address
}

int main() {
    
    char buffer[5000] { 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f } ;
    std::cout << "\n\nbuffer: " << pvoid(buffer) << "  buffer+1: " << pvoid(buffer+1) << "  buffer+6: " << pvoid(buffer+6) << "\n\n" << std::flush ;
    
    foo(buffer+1) ;
    bar(buffer+6) ;
}

buffer: 0x7fffc719ec20  buffer+1: 0x7fffc719ec21  buffer+6: 0x7fffc719ec26

*** foo called with buffer at: 0x7fffc719ec21
main.cpp:13:45: runtime error: load of misaligned address 0x7fffc719ec21 for type 'uint16_t', which requires 2 byte alignment
0x7fffc719ec21: note: pointer points here
 00 00 00  0a 0b 0c 0d 0e 0f 0a 0b  0c 0d 0e 0f 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
              ^ 
main.cpp:13:15: runtime error: store to misaligned address 0x7fffc719ec21 for type 'uint16_t', which requires 2 byte alignment
0x7fffc719ec21: note: pointer points here
 00 00 00  0a 0b 0c 0d 0e 0f 0a 0b  0c 0d 0e 0f 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
              ^ 
*** bar called with buffer at: 0x7fffc719ec26
main.cpp:21:33: runtime error: load of misaligned address 0x7fffc719ec26 for type 'uint32_t', which requires 4 byte alignment
0x7fffc719ec26: note: pointer points here
 0b 0d 0e 0f 0a 0b  0c 0d 0e 0f 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00
             ^ 
main.cpp:21:13: runtime error: store to misaligned address 0x7fffc719ec26 for type 'uint32_t', which requires 4 byte alignment
0x7fffc719ec26: note: pointer points here
 0b 0d 0e 0f 0a 0b  0c 0d 0e 0f 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00
             ^ 

http://coliru.stacked-crooked.com/a/4266a836093c6cb1

[zbjornson]

Thanks JLBorges, useful to see the output with that compiler option.