Hacker News Reader

> Adding a volatile to fix a threading bug is just a big no-no.

The fence instructions also don't make much sense. Not sure why one would not use std::atomic in a c++17 only library.

You'd think so, but in fact reinterpret_cast from char* (and unsigned char, and std::byte) is explicitly allowed by the type aliasing rules.

To the best of my knowledge, casting to char* is totally fine (inspecting an object as bytes) under certain constraints.

What is not fine is pretending that an object lives at a position in memory where it does not (i.e. treating bytes of memory as some object through a reinterpret_cast away from char* ).

Edit: To clarify, casting away from char* is of course allowed if you cast to whatever object type actually lives at the pointed-to location.

If I remember correctly, it is fine if there actually was an object of that type (or a "similar" type) in that location. So casting to char* and back is fine, casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ...

It seems like the function only cast to T* if the input pointer hasn't been changed (the paths that do modify it return early), so there's a value there?

> casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly

It is only fine if there were the same exact int types at that address. Remember, UB usually isn't the reinterpret_cast but the dereference.

I thought the int-types had a special exception making that always possible (if there's a bunch of bytes, you can turn them into an int), but I could very well be wrong on that detail.

Char is the only exception.

And now std::byte IIRC

> it is fine if there actually was an object of that type (or a "similar" type) in that location.

That is correct. See my other nearby reply.

> So casting to char* and back is fine

Indeed.

> casting to char and then to int-type to inspect multiple bytes is fine as long as alignment plays correctly, ...

Only if the place in memory you are pointing to actually has a live int object. In particular, inspecting the byte representation of an object is fine (including copying the bytes elsewhere, which is why they could just use memcpy instead of reinterpret_cast).

See also http://eel.is/c++draft/basic.types#2 and surroundings.

I had thought that, as long as the object in that location is valid (e.g. constructed with placement new and not since destructed, with the `char` as a pointer to the storage), then accessing it through the `char` is valid -- but maybe that's only true for `void*` now that I think about it. Either way, fair enough, placement new probably hasn't happened (haven't read the rest of the code).

(i.e. treating bytes of memory as some object through a reinterpret_cast away from char ).*

That's how people have used C and C++ for decades in low-level work, and it still works exactly as you'd expect, so stop saying "don't do it" because you're only encouraging the compiler-writer-UB-optimisation-nonsense crowd to make things even worse. It's already bad enough that they think the Holy Standard is the only thing that matters.

I believe Linus has several memorable rants on this topic already.

Doesn't matter anymore, Clang and GCC will both make any and all optimizations they can find by assuming you will never hit UB, including just erasing all UB code. You can dislike it, but that's just the reality now, and ignoring it won't make it go away. The only way to change it would be a change in the standard.

char* is universal memory format, you could specialize from that into anything you want

That's not how the C++ object model works. Objects in C++ have clearly defined lifetimes. If you reinterpret_cast<int* >(someCharBuffer) and then dereference that, you have Undefined Behavior unless an int object is alive at that exact location.

You can do

    int value[2] = {0, 0};
    char* ptr = reinterpret_cast<char*>(value) + sizeof(int);
    (*reinterpret_cast<int*>(ptr))++;

or (given knowledge about compiler padding):

    struct XY { double x; int y; };
    XY xy = {};
    char* ptr = reinterpret_cast<char*>(&xy) + sizeof(double);
    (*reinterpret_cast<int*>(ptr))++;

or even (placement new):

    unsigned char* buf = new unsigned char[12];
    buf += 4;
    new (buf) int(0); // Create an int object in the buffer.
    (*reinterpret_cast<int*>(buf))++;
    delete[] buf;

But in each case an int object has had its lifetime begin before you can treat the bytes in memory like an int. Everything else is UB.

You cannot do the last unless you know that the argument of placement new is suitably aligned to receive an int object, however.

What's your problem with the volatile? Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used.

The bug was that one thread was reading the gcc thought it wouldn't be update between reads. It is even used properly by grabbing a copy of the volatile value instead of reading it repeatedly.

edit: I also don't understand your aliasing issues. A `char*` can point anything and you are allowed to cast back out of it to the correct type. It is casting to int because the string is prepended by its length (the var names are "size" and "nibble").

> Using it to prevent register caching of or a value or prevent optimizing out repeated reads is perfectly fine. That is how it is supposed to be used.

The code has a race condition, plain and simple. volatile does not lead to thread synchronization, and that function is (as gcc correctly identified) nonsensical in a single-threaded world. The code expects that a different thread writes to the variable at the same time that this thread might read from it - that's the definition of a race condition, which is straight up UB (volatile or not).

You are trying to reason about compiler optimizations from the perspective of the machine and memory model you know ("prevent register caching of a value" etc.). That's wrong though - compiler optimizations operate on the abstract machine that C++ is defined on. In the abstract machine, this code has a race condition (the absence of which is guaranteed to compilers by the C++ standard) and so optimizations could easily violate whatever mental model you are using to conclude the code is fine. The code will likely break again some time in the future, just like it broke this time for a newer compiler version. Or maybe it compiles to subtly broken code right now, who knows!

> I also don't understand your aliasing issues. A `char* ` can point anything and you are allowed to cast back out of it to the correct type.

int is not the "correct type" because no int object lives at the pointed-to location after the cast. They could just memcpy and everything would be fine, but writing to/reading from type aliased pointers (other than char* and friends, in specific circumstances) is UB.

Here is a good description (in the context of C, not C++) of strict aliasing: https://stackoverflow.com/questions/98650/what-is-the-strict...

> The code expects that a different thread writes to the variable at the same time that this thread might read from it - that's the definition of a race condition,

No, that isn't. In one thread it reads a value over and over. Another thread will periodically set it. This isn't a race condition and requires no synchonization. On writer thread only, and you're fine. If the code was doing more, I didn't see it.

I just wrong something very similar yesterday where I had a counter that is read over and over and an async handler updated it. I had to make it volatile to prevent the the spin on it from optimizing out the load.

Yes, in low level programming, you have to know about the hardware, what a register is, what cache it, etc. C++ has a very lose (almost nonexistent) abstract model. This isn't UB.

> int is not the "correct type" because no int object lives at the pointed-to location after the cast

The string had its length prepended to it. There was an int there. Also, those aliasing rules are more about optimization issues, not as much about alignment issues (eg, if you are only writing for intel/amd hardware alignment only really matter for SIMD, and there is no penalty on unaligned access (with a few small exceptions that didn't apply here).

> No, that isn't. In one thread it reads a value over and over. Another thread will periodically set it. This isn't a race condition and requires no synch[r]onization. On[e] writer thread only, and you're fine.

Sorry, wrong term by me. I meant data race, not race condition (although many would regard the former as a form of the latter). A data race is two or more threads accessing the same location without synchronization, where at least one of them is modifying the value. A data race results in undefined behavior. Here are the relevant sections from the C++ (draft) standard:

http://eel.is/c++draft/intro.multithread#intro.races-21

http://eel.is/c++draft/intro.multithread#intro.races-2

> I just wro[te] something very similar yesterday where I had a counter that is read over and over and an async handler updated it.

If you wrote this code in C++, you created a data race and your program has undefined behavior.

> I had to make it volatile to prevent the the spin on it from optimizing out the load.

volatile may prevent the load from being optimized out, but it does not prevent the data race (because it does not prevent instruction reordering). Your program still has undefined behavior.