When I memcpy a struct into a std::atomic of that struct, why does the result not match?


Consider the following code:

// Code in italics is wrong.

struct Point3D { float x, y, z; };

std::atomic<Point3D> currentPoint;

bool LoadCurrentPointFromFile(HANDLE file)
{
 DWORD actualBytesRead;
 if (!ReadFile(file, &currentPoint, sizeof(Point3D),
               &actualBytesRead, nullptr)) return false;
 if (actualBytesRead != sizeof(Point3D)) return false;
 return true;
}

This code tries to load a Point3D structure from a file directly into a std::atomic. However, the customer found that the results were not properly loaded and suspected there may a bug in the Read­File function, because the value that should have been in the z member ended up in y, the value that should have been in the y member ended up in x, and the value that should have been in the x member wasn't loaded at all.

The Read­File function is working fine. What's wrong is that you aren't using the std::atomic variable properly.

The contents of a std::atomic variable are not directly accessible. You have to use methods like store and load. There are operator overloads which make atomic variables appear to be regular variables, but at no point can you get the address of the underlying Point3D storage.

Processors have restrictions on the sizes of operands on which they can natively perform atomic operations. Some restrictions apply to the size of the operand: Most processors do not support atomic operations on 12-byte objects, and it's not reasonable to expect a processor to be able to perform an atomic operation on a memory object that is megabytes in size, after all. Some restrictions are based on layout, such as whether the object is suitably aligned.

In the cases where the object cannot be managed atomically by the processor, the standard library steps in and adds a lock, and operations on the atomic variable take the lock to ensure that the operation is atomic. The reason everything is shifted is that the code took the address of the atomic variable itself, which includes the intenral lock, and the value you intended to read into x didn't vanish. It overwrote the lock!

Access to the contents of the atomic variable must be done by the appropriate methods on the atomic variable.

bool LoadCurrentPointFromFile(HANDLE file)
{
 DWORD actualBytesRead;
 Point3D point;
 if (!ReadFile(file, &point, sizeof(Point3D),
               &actualBytesRead, nullptr)) return false;
 if (actualBytesRead != sizeof(Point3D)) return false;
 currentPoint.store(point);
 return true;
}

There's a presentation from CppCon 2017 that covers std::atomic from start to finish, including performance characteristics. I'm going to consider this video to be homework, because next time I'm going to chatter about it.

Comments (16)
  1. Antonio Rodríguez says:

    I’m no expert in C++, and most of its runtime library is uncharted territory to me. But when I read the code, it was evident it was a type mismatch: they were taking an struct and copying it into an object. Of course, that object contains a copy of the struct as a member. If you are lucky (or not?), it will be the first member, and all will go well. But you are relying in an implementation detail that can change in the future.

    It seems that the atomic contains just an integer-sized lock and the target struct, so you could obtain a pointer to the struct with some pointer arithmetic. But it definitely would be a terrible idea. Not only can the implementation details change in future versions of the runtime. Also, accessing directly to an atomic’s contents does defeat it sole purpose: warrantying atomic operations.

    1. 'k says:

      I suppose the hope was that atomic is simply C++ language markup for the compiler to do some special joo joo by using atomic instructions while keeping the memory layout the same.

      1. Scarlet Manuka says:

        It’s presumably a case of starting off with a plain Point3D, getting everything working, and then discovering weird multithreaded behaviour. Then when they decided they need to use std::atomic, they didn’t realise that the way they used it would have to change in some places – as you said, probably because they thought of std::atomic as “tell the compiler to make all access to this variable atomic” and didn’t think about the technicalities. And while I’m no C++ expert, one thing I have picked up is that the technicalities will always come back to bite you.

    2. Neil says:

      Well, they were reading into an object but not using its size, so that should have been a red flag already.

  2. mikeb says:

    I don’t understand how anyone who would need an atomic type could think something like that would work.

    1. Kevin says:

      This is the “wave a magic wand over the problem” school of thought. Whoever wrote that code didn’t think about what std::atomic actually means or how it might be implemented. Instead, they simply think of it as a black box that “magically fixes all of my threading bugs.”

      (This is, of course, complete hogwash: you can easily have threading bugs even if everything is atomic, because threading bugs arise from faulty assumptions about the sequencing of operations. std::atomic makes things easier to reason about, but it does not eliminate the need to think.)

    2. Peter says:

      Yeah. That had me scratching my head too. Even if you could get the correct target address, the read operation would not be atomic.

    3. Because it does work (for certain values of “work”) sometimes. If this were a Point2D instead of a Point3D, there’s a good chance it would have “worked”.

      1. Peter Doubleday says:

        It isn’t really a std::atomic problem as such. It’s an even lower level failure to understand the rules of the C++ memory model.
        The target variable in ReadFile() is an LPVOID, which should be easy enough for any MS C++ programmer to understand. All you can expect to get out of this is a pointer/reference to a contiguous chunk of memory that (hopefully, error result permitting) has the same size in bytes that you requested. It’s up to you to figure out what those (in this case) twelve bytes mean, because ReadFile() has no possible way of even giving you a hint. The fact that the requested size is “sizeof(Point3D)” rather than “sizeof(placeholder for std::atomic)” should advert the caller to an obvious container mismatch.
        You could actually pick any other STL container and attempt to apply this “trick,” and the result would be equally as useless. Effectively you are implicitly casting between two separate types via a void pointer, and any C++ programmer who thinks this is a good idea … should not be programming in C++.

        1. I think ‘k was right : They probably thought that atomc<T> is physically the same as T; the atomicity was believed to be entirely encapsulated in the algorithm (e.g., “interlocked operations everywhere!”), with no additional data needed.

      2. Billy O'Neal says:

        I think we are going to guarantee that it does work as long as atomic::is_always_lock_free in a future release but we don’t do that right now. (We want atomic to be in the “ABI stabilized” domain as quickly as we can)

  3. Billy O'Neal says:

    Note that sizeof(atomic<T>) can be surprising in relation to sizeof(T). For example, in our implementation, if you put a 3 byte struct into an atomic we don’t round up, we go “that’s not one of the sizes we recognize” and add our lock, making sizeof(atomic<bytes>) == 8.

    We fixed that bug in the next ABI-breaking release of the Visual C++ libraries but the date we get to ship that seems to only always get further away :/

  4. Ismo says:

    The ReadFile can never operate on something which is more complex than pod (plain old data). I’ve seen memset(*this,0) stl code and it really crashes wierd ways. C++ is powerfull tool, it lets you to shoot your feet if not handled properly.
    I doubt that nobody reading Raymond’s blog would make that mistake.

  5. Andreas says:

    Hehe, that developer needs to eat some more humble pie. *If* there was a bug in ReadFile as easily exposed as this, that means millions of programs on >1 billion devices are affected. So the outcry should be enormous. If you only hear *crickets* that *probably* means your own code is at fault. When in doubt blame yourself!

    1. Joshua says:

      On the other hand I found a real bug in System.Data.SqlClient that was otherwise unknown for two years. If you express a preference for isolation levels on some open connections, ones that you didn’t do so end up with a random isolation level due to a dirty pool.

  6. Phont Madders says:

    “Code in italics is wrong.”
    That’s why I only code in boldface.

Comments are closed.

Skip to main content