Fix a violation of C++ standard rules where unions are used for type-punning #18167

Type punning in C++ is not like C. As per the C++ standard, one cannot use unions to convert the bit type. A discussion about this can be found here. In C++, a union is supposed to only hold one type at a time. It's intended to be used only as std::variant. Switching types is undefined behavior.

In fact, C++20 has a special casting function, called bit_cast that solved this problem.

Why has it been working so far? Because some compilers tolerate using unions and switching types, like gcc. More information here.

One important thing to mention is that performance is generally not affected by that memcpy. Compilers are smart enough to convert that to a memory cast when possible. But we have to do it the right way, otherwise, it's jut undefined behavior that depends on the compiler.

Great first-time contribution! Welcome as a contributor! Hope to see more contributions from you.

Thanks for tackling UB issues in the project. The bulk of them should be fixed by now and this is one of the last few UB issues I'm aware of. Don't hesitate to report and/or fix any other UB issues you might find and don't hesitate to ping me if you want your work reviewed.

Concept ACK

FWIW:

1. The C++ Core Guidelines – with editors Bjarne Stroustrup and Herb Sutter – has this to say: "It is undefined behavior to read a union member with a different type from the one with which it was written."

2. The C++ Core Guidelines has this to say: "Unfortunately, unions are commonly used for type punning. We don’t consider “sometimes, it works as expected” a strong argument."

3. Professor John Regehr has this to say: "Again, although we might expect that adding a function call would make the code harder to optimize, both compilers understand memcpy deeply enough that we get the desired object code […] In my opinion c5 is the easiest code to understand out of this little batch of functions because it doesn’t do the messy shifting and also it is totally, completely, obviously free of complications that might arise from the confusing rules for unions and strict aliasing. It became my preferred idiom for type punning a few years ago when I discovered that compilers could see through the memcpy and generate the right code."

4. Video recommendation: Timur Doumler's talk “Type punning in modern C++” from CppCon 2019

ACK be94096dfb0c4862e2314cbae4120d7360b08ef2

Verified that clang++ -O1 (trunk) generates exactly the same object code before and after the change (as expected) for each of the changed functions:

<details><summary>Click for results</summary>

#include <cstdint>
#include <cstring>

uint64_t ser_double_to_uint64_old(double x)
{
    union { double x; uint64_t y; } tmp;
    tmp.x = x;
    return tmp.y;
}

uint64_t ser_double_to_uint64_new(double x)
{
    uint64_t tmp;
    std::memcpy(&tmp, &x, sizeof(x));
    return tmp;
}

uint32_t ser_float_to_uint32_old(float x)
{
    union { float x; uint32_t y; } tmp;
    tmp.x = x;
    return tmp.y;
}

uint32_t ser_float_to_uint32_new(float x)
{
    uint32_t tmp;
    std::memcpy(&tmp, &x, sizeof(x));
    return tmp;
}

double ser_uint64_to_double_old(uint64_t y)
{
    union { double x; uint64_t y; } tmp;
    tmp.y = y;
    return tmp.x;
}

double ser_uint64_to_double_new(uint64_t y)
{
    double tmp;
    std::memcpy(&tmp, &y, sizeof(y));
    return tmp;
}

float ser_uint32_to_float_old(uint32_t y)
{
    union { float x; uint32_t y; } tmp;
    tmp.y = y;
    return tmp.x;
}

float ser_uint32_to_float_new(uint32_t y)
{
    float tmp;
    std::memcpy(&tmp, &y, sizeof(y));
    return tmp;
}

…

ser_double_to_uint64_old(double):          # [@ser](/bitcoin-bitcoin/contributor/ser/)_double_to_uint64_old(double)
        movq    rax, xmm0
        ret
ser_double_to_uint64_new(double):          # [@ser](/bitcoin-bitcoin/contributor/ser/)_double_to_uint64_new(double)
        movq    rax, xmm0
        ret
ser_float_to_uint32_old(float):           # [@ser](/bitcoin-bitcoin/contributor/ser/)_float_to_uint32_old(float)
        movd    eax, xmm0
        ret
ser_float_to_uint32_new(float):           # [@ser](/bitcoin-bitcoin/contributor/ser/)_float_to_uint32_new(float)
        movd    eax, xmm0
        ret
ser_uint64_to_double_old(unsigned long):          # [@ser](/bitcoin-bitcoin/contributor/ser/)_uint64_to_double_old(unsigned long)
        movq    xmm0, rdi
        ret
ser_uint64_to_double_new(unsigned long):          # [@ser](/bitcoin-bitcoin/contributor/ser/)_uint64_to_double_new(unsigned long)
        movq    xmm0, rdi
        ret
ser_uint32_to_float_old(unsigned int):           # [@ser](/bitcoin-bitcoin/contributor/ser/)_uint32_to_float_old(unsigned int)
        movd    xmm0, edi
        ret
ser_uint32_to_float_new(unsigned int):           # [@ser](/bitcoin-bitcoin/contributor/ser/)_uint32_to_float_new(unsigned int)
        movd    xmm0, edi
        ret

</details>

~0

Seems like the better solution here is to stop assuming floats have a specific internal representation? :/

@luke-jr That would indeed be preferable, but it seems that would effectively require implementing a IEEE 754 software encoder/decoder, which is nontrivial.

This is still an improvement though. memcpy into another type seems indeed to be the modern idiom (we use the same in ReadLE32 and friends, after verifying that compilers indeed optimize through that).

Concept ACK

@luke-jr That would indeed be preferable, but it seems that would effectively require implementing a IEEE 754 software encoder/decoder, which is nontrivial.

There's a famous library, called softfloat. It does all that. But I think that's unnecessary. IEEE 754 is the same everywhere as repsentation. The differences between different CPUs come from certain operations, like rounding and division by epsilon (IIRC). You can see in softfloat different rounding modes and similar things.

ACK 0653939ac130eddffe40c53ac418bea305d3bf82

ACK 0653939ac130eddffe40c53ac418bea305d3bf82

ACK 0653939ac130eddffe40c53ac418bea305d3bf82

Code review ACK 0653939ac130eddffe40c53ac418bea305d3bf82

That would indeed be preferable, but it seems that would effectively require implementing a IEEE 754 software encoder/decoder, which is nontrivial.

Please, don't. If you're really willing to pick up a huge amount of work to support obscure platforms, I would prefer to move away from serializing floating point numbers at all.I tried this once but there's quite some usage (luckily, not in the P2P or consensus, only for internal file formats, so it's not impossible at least!).

Great to see this merged! @TheQuantumPhysicist If you want to tackle the few remaining instances of UB you might want to build with CC=clang CXX=clang++ ./configure --with-sanitizers=undefined && make and fix any violations you are able to trigger :)

@practicalswift Thanks for the support! Actually I use clang sanitizers all the time. Thread sanitizer, undefined behavior sanitizer, memory sanitizer and address sanitizer. You can see them in all my github projects that I wrote myself. But this one here I discovered by coincidence and hence fixed 😉

And I almost never use gcc anymore 😄

Btw, if you're interested, there's an issue I opened in libbtc about undefined behavior like 2 years ago... it's still untouched and not merged.

Checked that be94096dfb0c4862e2314cbae4120d7360b08ef2 compiles to the same bitcoind with -O2 on:

• clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) Target: aarch64-unknown-linux-gnu

• clang version 9.0.0 (Fedora 9.0.0-1.fc31) Target: x86_64-unknown-linux-gnu