points by mehrdadn 6 years ago

It sounds like you don't get what the article's point is. The article is NOT using volatile as a barrier mechanism. It's using it as a compiler-enforced type annotation, which you strip away before accessing the variable of interest. It sounds like absolutely nobody here is willing to read the article because they think they already know everything the article could possibly be saying. Fine, I give up, you win. I've summarized it here for you.

The idea is this you can use volatile like below. It's pretty self-explanatory. Now can you look through this code and tell me where you see such a horrifying lack of memory barriers and undefined behavior? (And don't point out something irrelevant like how I didn't delete the copy constructor.)

  #include <mutex>

  template<class T>
  class lock_ptr
  {
      T *p;
  public:
      ~lock_ptr() { this->p->m.unlock(); }
      lock_ptr(volatile T *p) : p(const_cast<T *>(p)) { this->p->m.lock(); }
      T *operator->() const { return p; }
  };

  class MyClass
  {
      int x;
  public:
      MyClass() : x() { }
      mutable std::mutex m;
      void bar() { ++x; }
      void foo() volatile { return lock_ptr<MyClass>(this)->bar(); }
  };

  void worker(volatile MyClass *p)  // called in multiple threads
  {
      p->foo();  // thread-safe, and compiles fine
      p->bar();  // thread-unsafe, and compile-time error
  }

  #include <future>

  int main()
  {
      MyClass c;
      auto a = std::async(worker, &c);
      auto b = std::async(worker, &c);
      a.wait();
      b.wait();
      return c.x;
  }
ChrisLomont 6 years ago

> It sounds like you don’t get what the articles point is.

Yes I do. It’s simply wrong. What it says about type annotation is correct, but has zero to do with threading because volatile has zero meaning for accesses from different threads. It then uses volatile to (incorrectly) build threading code. You seem to think volatile has some usefulness for threaded code; it does not. You think volatile adds benefit to your code above; it does not. The type annotation does not give you the ability to have compilers check race conditions for you - it works on some and will fail on others.

Add volatile to your bar function. Oops, got race conditions. Volatile is not protecting your code; properly using mutexes is. Requiring programmers to intersperse volatile as some type annotation makes code more error prone, not less. One still has to correctly do the hard parts, but now with added confusion, verbosity, and treading on undefined behavior.

I think you believe his claim “We can make the compiler check race conditions for us.” because you’re relying on the same claim compilers will check volatile in the manner your code above does. That’s undefined behavior, open to compiler whims. Good luck with that. There’s a reason C++ added the more nuanced ordering specifications - to handle the myriad ways some architectures worked (and to mirror discoveries made in academic literature on the topic that happened after this article was written).

This article is even mentioned in the proposal to remove volatile from C++ altogether http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p115.... I’ve known about this for some time, and hacking in type annotations like this adds no value; it simply makes a mess.

More errors from the article, which is why people should stop citing it:

First sentence:

“The volatile keyword was devised to prevent compiler optimizations that might render code incorrect in the presence of certain asynchronous events.”

This is simply wrong. The article goes on to try to make multithreaded code correct using volatile.

More quotes from the article that are simply wrong: “Although both C and C++ Standards are conspicuously silent when it comes to threads, they do make a little concession to multithreading, in the form of the volatile keyword.” Wrong; see Sutter quote below. “Just like its better-known counterpart const, volatile is a type modifier. It's intended to be used in conjunction with variables that are accessed and modified in different threads.” Wrong. See Sutter quote, and ISO standards. Volatile was never intended for this, so was never safe for doing this. “In spite of its simplicity, LockingPtr is a very useful aid in writing correct multithreaded code. You should define objects that are shared between threads as volatile” wrong on so many levels. The referenced code will break on many, many architectures. There is simply no defense to this.

The article has dozens more incorrect statements and code samples trying to make threadsafe code via volatile.

I’ve written articles on this. I’ve taught professional programmers this. I’ve designed high performance C++ multithreaded code for quite a while. It’s simply wrong, full stop.

Here’s a correct destruction of the Dobbs article by someone who gets it [1]. They, like you, were once misled by this article.

The money quote, from Herb Sutter “Please remember this: Standard ISO C/C++ volatile is useless for multithreaded programming. No argument otherwise holds water; at best the code may appear to work on some compilers/platforms”

I suspect you’ll still stick to the claim this article has value, given your insistence so far against so many people giving you correct advice. Good luck.

[1] https://sites.google.com/site/kjellhedstrom2/stay-away-from-...

  • the_why_of_y 6 years ago

    > “The volatile keyword was devised to prevent compiler optimizations that might render code incorrect in the presence of certain asynchronous events.” > This is simply wrong.

    Hardware interrupts and UNIX signals are the asynchronous events in question, and C's volatile is still useful in those contexts, where there is only a single thread of execution.

    • ChrisLomont 6 years ago

      Volatile still doesn’t protect you there, whereas C++ 11 atomic do. If the item you mark volatile is not changed at the cpu and cache level atomically, you’re going to access torn variables. I’ve been there and am certain about it. And pre C++ 11, there is no way to portably find out which operations are architecture atomic, so it was impossible to write such code portably. C++ 11 fixed all that, and there’s no reason to use volatile for any of this any more: use atomics, possibly with fine grained barriers if needed and understood.

      Here’s a compiler showing that your use fails on some systems:

      http://www.keil.com/support/docs/2801.htm

      • the_why_of_y 6 years ago

        You're right that just "volatile" isn't enough; typically you'd declare the variable sig_atomic_t to be portable, which makes the necessary guarantees since C89 so predates C++11. (It does not guarantee anything regarding access from multiple threads, of course.)

        The problem with std::atomic<T> is that it may be implemented with a mutex, in which case it can deadlock in a signal handler. But as you say, you can check for that with is_lock_free.

        • ChrisLomont 6 years ago

          Yep. And this thread illustrates why threading is hard, especially in C++ :)

          Oh, and sig_atomic_t is not guaranteed thread-safe, only signal safe. The difference is when you move your code from a single cpu to dual cpu system it breaks. I ran across this some time ago moving stuff to an ESP32.

          Atomic so far works best across the chips I’m poking at.