ConfigurationBlobFlash::BlobIsValid() could give the wrong result

[zhmu]

I'm using gcc 9.3.1 for arm, as bundled with STM32CubeIDE 1.6.1 (gcc version 9.3.1 20200408 (release) (GNU Tools for STM32 9-2020-q2-update.20201001-1621)). When trying to build with -O3 -Werror, I get a warning regarding the following code:

bool ConfigurationBlobFlash::BlobIsValid() const
{
    Header header;
    infra::Copy(infra::Head(blob, sizeof(header)), infra::MakeByteRange(header));

    if (header.size + sizeof(Header) > blob.size())
        return false;

where the warning is:

'header.services::ConfigurationBlobFlash::Header::size' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     76 |         if (header.size + sizeof(Header) > blob.size())
        |             ~~~~~~~^~~~

I believe this warning to be correct and the code to be wrong: by using infra::MakeByteRange, header will be cast to an uint8_t*, which will be used by infra::Copy (which seems to call std::copy internally) to fill the byte range.

However, due to the strict aliasing rule, the compiler is free to assume that the uint8_t* being written does not overlap with header as they have different types. I suspect it might even be legal to optimize away the copy completely as there are no observable side-effects within the rules of the language.

My recommendation would be to simply do the following:

std::memcpy( &header, blob.begin(), sizeof( header ) );

which will yield the desired result with no risk of causing troubles.

(There are more constructs like these, it may be worthwhile to build with -O3 -Werror to locate them)

[daantimmer]

I would rather suggest replacing that infra::Copy(...) with a std::bit_cast<Header>(blob).
Although std::bit_cast is only introduced in C++20, there is a C++17 implementation possible, see Possible Implementation on the cppreference page: https://en.cppreference.com/w/cpp/numeric/bit_cast#Possible_implementation

template <class To, class From>
std::enable_if_t<
    sizeof(To) == sizeof(From) &&
    std::is_trivially_copyable_v<From> &&
    std::is_trivially_copyable_v<To>,
    To>
// constexpr support needs compiler magic
bit_cast(const From& src) noexcept
{
    static_assert(std::is_trivially_constructible_v<To>,
        "This implementation additionally requires "
        "destination type to be trivially constructible");
 
    To dst;
    std::memcpy(&dst, &src, sizeof(To));
    return dst;
}

[richardapeters]

I don't think converting all instances where we violate the strict aliasing rules to use std::bit_cast or memcpy gives satisfactory results. In lots of cases, we have an object of which the contents is read from flash or spi or something like that:

void PackUpgrader::UpgradeFromImages(...)
{
UpgradePackHeaderPrologue headerPrologue;
upgradePackFlash.ReadBuffer(infra::MakeByteRange(headerPrologue), address);

Using either bit_cast or memcpy in the flash driver's ReadBuffer would mean that first we need an additional buffer of sufficient size, read the contents from flash into that buffer, and then copy that into headerPrologue. That additional buffer is a real problem: its size is unknown up front, and it might need to be pretty big, depending on what kind of object we are reading from flash. It might be a bunch of 4kB certificates.

If I understand the strict aliasing rules correctly, then we cannot use uint8_t* because when writing via such a pointer the compiler does not need to acknowledge the possibility that the original object is modified. However, using char, unsigned char, or std::byte seems to be allowed. I read these relevant portions of the standard:

6.8 Types
2 For any object (other than a potentially-overlapping subobject) of trivially copyable type T, whether or not
the object holds a valid value of type T, the underlying bytes (6.7.1) making up the object can be copied into
an array of char, unsigned char, or std::byte (17.2.1).36 If the content of that array is copied back into
the object, the object shall subsequently hold its original value. [Example:
constexpr std::size_t N = sizeof(T);
char buf[N];
T obj; // obj initialized to its original value
std::memcpy(buf, &obj, N); // between these two calls to std::memcpy, obj might be modified
std::memcpy(&obj, buf, N); // at this point, each subobject of obj of scalar type holds its original value
— end example]

7.2 Properties of expressions, 7.2.1 Value category
11 If a program attempts to access (3.1) the stored value of an object through a glvalue whose type is not
similar (7.3.5) to one of the following types the behavior is undefined:51
(11.1) — the dynamic type of the object,
(11.2) — a type that is the signed or unsigned type corresponding to the dynamic type of the object, or
(11.3) — a char, unsigned char, or std::byte type.

3.1 Access
〈execution-time action〉 read (7.3.1) or modify (7.6.19, 7.6.1.5, 7.6.2.2) the value of an object

So my expectation is that when we use std::byte instead of uint8_t for all our byte-accesses, that should solve the normal part of violating strict aliasing rules.

Open question: Does this solve accesses via DMA as well? What if flash's ReadBuffer is implemented with DMA()? The DMA is then initialized with a std::byte*, and after the DMA finishes we execute a __DMB(). I sincerely hope that compilers will accept that construct.

@zhmu What do you think of this?

[zhmu]

The compiler isn't required to do the actual memcpy() if it can prove that it is not necessary to do so. I've done some experimentation with GCC 12.2 on x86-64 and ARM. On ARM the relevant variables are copied as the compiler cannot prove that the blob buffer is properly aligned. This doesn't seem to happen on x86-64, likely because the cost of a potential unaligned access is lower than making the copy, but that is just (my) guesswork.

I think using std::byte would be a reasonable approach. The compiler doesn't specifically care about DMA and such, it is up to you to use the correct machinery to ensure the buffer is valid once you start using it.

On a sidenote, as long as uint8_t == unsigned char (which seems to be the case on GCC ARM), you should be in the clear - however, this is not required by the standard. Perhaps it is worthwhile to add a static_assert( std::is_same_v<uint8_t, unsigned char> ); until things are converted to std::byte.

Finally, since uint8_t is identical to unsigned char on GCC ARM, I'm confused why I am getting this warning at all, as it should be well-defined in this case.