Fix for the Bug 41784

[rarutyun]

Summary

Add deleted volatile copy-assignment operator in the most derived atomic to fix the Bug 41784.

The root cause: there is an operator=(T) volatile that has better match than the deleted copy-assignment operator of the base class when this is volatile. The compiler sees that right operand of the assignment operator can be converted to T and chooses that path without taking into account the deleted copy-assignment operator of the base class.

The current behavior on libstdc++ is different from what we have in libc++. On the same test compilation fails with libstdc++. Proof: https://godbolt.org/z/nebPYd (everything is the same except the -stdlib option).

I choose the way with explicit definition of copy-assignment for atomic in the most derived class. But probably we can fix that by moving operator=(T) overloads to the base class from both specializations. At first glance, it shouldn't break anything.

[zoecarver]

libcxx/include/atomic

1866

Correct me if I'm wrong, but I don't think we need this. Clang in C++03 mode supports these.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/copy.assign.volatile.fail.cpp

22

Can you provide an expected-error comment? Look at other .fail tests for examples.

[rarutyun]

libcxx/include/atomic

1866

I am not sure. I've checked it and it works without that macro check but I did that for some subset of compilers. I took into account both GCC and clang. I suspect it may give some warnings on some compilers but on the other hand we have #pragma GCC system_header, so I am not sure how to prove the necessity of that branch. I also tried to reproduce some strange behavior by playing with -pedantic option but with no luck.

The reason why I add that check is it initially was in __atomic_base class and it's still there for copy-constructor. Unless we can prove this check is unnecessary and everything would work on all platforms with any compiler I would prefer to leave it as is.

[zoecarver]

libcxx/include/atomic

1866

Thanks for going to all the trouble of testing with several configurations. I'm actually pretty sure we don't need it. We use to support many more compilers (and namely compilers that didn't support deleting copy constructors in C++03 mode)-- that's why it was/still is present in parts of the codebase.

The only compiler we support in C++03 mode is Clang 4+, so, as I said, I'm pretty sure we're in the clear. Also, as long as the buildbots pass, I think we're OK (for future reference).

[rarutyun]

libcxx/include/atomic

1866

Ok, I have made that. Let's see if CI shows any problems with that change.

[ldionne]

libcxx/include/atomic

1866

Can you please rebase the patch onto main and re-upload the patch? It will trigger CI.

[rarutyun]

libcxx/include/atomic

1866

Done

[rarutyun]

Hi @ldionne,

Any progress on that review? Should I rebase it one more time?

[zoecarver]

This LGTM other than the small note.

libcxx/include/atomic

1791

Rather than saying "to fix the Bug..." do you mind saying something along the lines of Deleted explicitly in the most derived class to satisfy the requirement that atomic variables are not copyable.

1792

IIUC this line is a NFC? I think it makes sense to have these both here, just checking.

[rarutyun]

libcxx/include/atomic

1792

If my assumption is correct that NFC is "Not Functional Change")) then the answer is not exactly. Technically I think we can leave one overload but in that case user would get less informative compiler message (checked on clang). Depending on what overload we choose to leave the user may see either message about ambiguity of operator= or that atomic does not provide the copy-assignment with volatile qualifier.

With the current patch user sees clear message: "overload resolution selected deleted operator '='"

[zoecarver]

libcxx/include/atomic

1792

If my assumption is correct that NFC is "Not Functional Change")) then the answer is not exactly.

Yes, that's what I mean. I'm asking because if there is a new error message, maybe we should test it. But it doesn't look like there is...

Using the Godbolt link you sent, it looks like for non-volatile atomic variables, libc++ says:

error: overload resolution selected deleted operator '='
error: object of type 'std::atomic<int>' cannot be assigned because its copy assignment operator is implicitly deleted

So, the first part at least won't change.

Anyway, this still looks good to me. Thanks.

[rarutyun]

Hi @ldionne,

What about this patch? should I ping Olivier that he has a look?

[__simt__]

This change looks good to me, thanks!

[ldionne]

In addition to the comments I made, can you please add tests using the type traits like static_assert(!std::is_copy_constructible_v<std::atomic<int>>);?

libcxx/include/atomic

1680–1681

Do we have the exact same issue with copy-construction? The following works, but I don't think it should:

#include <atomic>

int main(int, char**) {
    volatile std::atomic<int> a1;
    std::atomic<int> a2(a1);
}

1791

I don't feel like this comment adds something useful.

1865

I don't feel like this comment adds something useful.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/copy.assign.ptr.volatile.fail.cpp

17–19 ↗

(On Diff #316102)

The signature of main must be int (int, char**) to satisfy freestanding compilation (where main isn't a special function).

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/copy.assign.volatile.fail.cpp

2

This test should be copy.assign.volatile.verify.cpp.

Tests ending in .verify.cpp are treated as Clang-verify tests. .fail.cpp tests too, but with special rules that we'd like to deprecate.

[rarutyun]

libcxx/include/atomic

1680–1681

The answer is weird:) Basically, we have and we don't. Yes, the code you provided works *and* it probably should.

In C++ standard for copy-assignment operator we have two overloads. One of them as explicitly marked volatile. Thus, for the test I've added the overload resolution should prefer copy-assignment volatile overload to everything else.

For the constructors we cannot have the overloads with cv-qualified for *this because *this doesn't exist yet (to my understanding). In C++ standard we have atomic (const atomic&) = delete; and that's it. If it's argument of copy-constructor is atomic or const atomic then it would choose deleted copy-constructor. volatile atomic cannot be matched to const atomic&. Possible fix is atomic (const volatile atomic&) = delete;

So, probably the issue is there but we should bring it to C++ standard committee if we believe this is the issue we need to fix. Currently everything works according to the spec. If the possible fix I wrote above would be taken in C++ standard then you are right and we would need to write the copy constructor in the most derived class instead of atomic_base

[rarutyun]

libcxx/include/atomic

1791

The mess in the comments was fixed. Or do you suggest something else? Should I remove the comment and just leave the reference to the bug or should I completely remove the comment?

1865

The mess in the comments was fixed. Or do you suggest something else? Should I remove the comment and just leave the reference to the bug or should I completely remove the comment?

[rarutyun]

In D90968#2507523, @ldionne wrote:

In addition to the comments I made, can you please add tests using the type traits like static_assert(!std::is_copy_constructible_v<std::atomic<int>>);?

Sure, will do it in the next update. I guess I should check both copy constructible and copy assignable there. Anything else?

[ldionne]

In D90968#2521256, @rarutyun wrote:

In D90968#2507523, @ldionne wrote:

In addition to the comments I made, can you please add tests using the type traits like static_assert(!std::is_copy_constructible_v<std::atomic<int>>);?

Sure, will do it in the next update. I guess I should check both copy constructible and copy assignable there. Anything else?

That's all I can think of. We could have checked for !std::is_constructible<std::atomic<T>, std::atomic<T> const volatile&>, but as you say, the spec seems to allow it right now.

libcxx/include/atomic

1680–1681

Ah, I see. So we're calling atomic::operator T() on a1, and then atomic<T>::atomic(T desired) to construct a2. @__simt__ Do you think this is expected, or should an issue be filed?

It certainly violates what I would have naively expected.

1791

I don't think the comment (even fixed) adds much value, but feel free to leave it there if you prefer. If someone's interested in knowing why we inherit here, they can git blame. Another way to see this -- if this had been written that way from the start (and a bug report never had to be filed), would this comment be there? I don't believe so. That's sort of the point I'm trying to make.

But as I said, if you think it's useful, feel free to keep it. I'm not strongly attached either way, it's just a comment.

[rarutyun]

That's all I can think of. We could have checked for !std::is_constructible<std::atomic<T>, std::atomic<T> const volatile&>, but as you say, the spec seems to allow it right now.

Done. I decided to do it for copy-constructor and both volatile and non-volatile overload of copy-assignment. And the same is done for pointer specialization for all three methods.

libcxx/include/atomic

1791

Ok, I see your point. I also don't have the strong opinion on that. That's basically what I am doing usually for unobvious fix but probably git blame is one of the options.

Anyway, comment has been removed

[ldionne]

I would still like to know what is @__simt__ 's opinion about the behavior of the copy constructor. Do you want us to file a LWG issue?

I'll ship this now, thanks.

libcxx/include/atomic

#endif
     _LIBCPP_INLINE_VISIBILITY
     __atomic_base() _NOEXCEPT _LIBCPP_DEFAULT
 
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
     __atomic_base(_Tp __d) _NOEXCEPT : __a_(__d) {}
 
 #ifndef _LIBCPP_CXX03_LANG
     __atomic_base(const __atomic_base&) = delete;
-    __atomic_base& operator=(const __atomic_base&) = delete;
-    __atomic_base& operator=(const __atomic_base&) volatile = delete;
 #else
 private:
     _LIBCPP_INLINE_VISIBILITY
     __atomic_base(const __atomic_base&);
-    _LIBCPP_INLINE_VISIBILITY
-    __atomic_base& operator=(const __atomic_base&);
-    _LIBCPP_INLINE_VISIBILITY
-    __atomic_base& operator=(const __atomic_base&) volatile;
 #endif
 };
 
 #if defined(__cpp_lib_atomic_is_always_lock_free)
 template <class _Tp, bool __b>
 _LIBCPP_CONSTEXPR bool __atomic_base<_Tp, __b>::is_always_lock_free;
 #endif
 
 // atomic<Integral>
struct atomic
     _LIBCPP_CONSTEXPR atomic(_Tp __d) _NOEXCEPT : __base(__d) {}
 
     _LIBCPP_INLINE_VISIBILITY
     _Tp operator=(_Tp __d) volatile _NOEXCEPT
         {__base::store(__d); return __d;}
     _LIBCPP_INLINE_VISIBILITY
     _Tp operator=(_Tp __d) _NOEXCEPT
         {__base::store(__d); return __d;}
+
+    // delete explicitly in the most derived class to fix the Bug 41784
+#ifndef _LIBCPP_CXX03_LANG
+    atomic& operator=(const atomic&) = delete;
+    atomic& operator=(const atomic&) volatile = delete;
+#else
+private:
+    _LIBCPP_INLINE_VISIBILITY
+    atomic& operator=(const atomic&);
+    _LIBCPP_INLINE_VISIBILITY
+    atomic& operator=(const atomic&) volatile;
+#endif
 };
 
 // atomic<T*>
 
 template <class _Tp>
 struct atomic<_Tp*>
     : public __atomic_base<_Tp*>
 {
struct atomic<_Tp*>
     _LIBCPP_INLINE_VISIBILITY
     _Tp* operator+=(ptrdiff_t __op) volatile _NOEXCEPT {return fetch_add(__op) + __op;}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* operator+=(ptrdiff_t __op) _NOEXCEPT          {return fetch_add(__op) + __op;}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* operator-=(ptrdiff_t __op) volatile _NOEXCEPT {return fetch_sub(__op) - __op;}
     _LIBCPP_INLINE_VISIBILITY
     _Tp* operator-=(ptrdiff_t __op) _NOEXCEPT          {return fetch_sub(__op) - __op;}
+
+    // delete explicitly in the most derived class to fix the Bug 41784
+#ifndef _LIBCPP_CXX03_LANG
+    atomic& operator=(const atomic&) = delete;
+    atomic& operator=(const atomic&) volatile = delete;
+#else
+private:
+    _LIBCPP_INLINE_VISIBILITY
+    atomic& operator=(const atomic&);
+    _LIBCPP_INLINE_VISIBILITY
+    atomic& operator=(const atomic&) volatile;
+#endif
 };
 
 // atomic_is_lock_free
 
 template <class _Tp>
 _LIBCPP_INLINE_VISIBILITY
 bool
 atomic_is_lock_free(const volatile atomic<_Tp>* __o) _NOEXCEPT

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/copy.assign.volatile.fail.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <atomic>
+
+// std::atomic
+
+// atomic& operator=( const atomic& ) volatile = delete;
+
+#include <atomic>
+
+int
+main()
+{
+    volatile std::atomic<int> obj1;
+    std::atomic<int> obj2;
+    obj1 = obj2;
+}
+