Page MenuHomePhabricator

Fix for the Bug 41784
ClosedPublic

Authored by rarutyun on Nov 6 2020, 1:08 PM.

Details

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.

Diff Detail

Event Timeline

rarutyun created this revision.Nov 6 2020, 1:08 PM
rarutyun requested review of this revision.Nov 6 2020, 1:08 PM
zoecarver added inline comments.Nov 28 2020, 11:47 AM
libcxx/include/atomic
1864

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 ↗(On Diff #303376)

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

rarutyun updated this revision to Diff 310251.Dec 8 2020, 9:33 AM
rarutyun marked an inline comment as done.Dec 8 2020, 9:40 AM
rarutyun added inline comments.
libcxx/include/atomic
1864

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 added inline comments.Dec 8 2020, 9:45 AM
libcxx/include/atomic
1864

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 updated this revision to Diff 310551.Dec 9 2020, 8:43 AM
rarutyun marked 2 inline comments as done.Dec 9 2020, 8:45 AM
rarutyun added inline comments.
libcxx/include/atomic
1864

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

curdeius set the repository for this revision to rG LLVM Github Monorepo.Dec 10 2020, 1:36 PM
curdeius added a project: Restricted Project.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptDec 10 2020, 1:36 PM
ldionne added inline comments.Dec 14 2020, 7:49 AM
libcxx/include/atomic
1864

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

rarutyun updated this revision to Diff 311611.Dec 14 2020, 8:47 AM
rarutyun marked an inline comment as done.
rarutyun marked an inline comment as done.
rarutyun added inline comments.
libcxx/include/atomic
1864

Done

rarutyun marked an inline comment as done.Dec 29 2020, 1:56 AM

Hi @ldionne,

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

zoecarver accepted this revision.Jan 11 2021, 5:06 PM

This LGTM other than the small note.

libcxx/include/atomic
1798

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.

1799

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

rarutyun updated this revision to Diff 316102.Jan 12 2021, 8:26 AM
rarutyun marked 2 inline comments as done.Jan 12 2021, 8:58 AM
rarutyun added inline comments.
libcxx/include/atomic
1799

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 '='"

rarutyun marked an inline comment as done.Jan 12 2021, 9:16 AM
zoecarver accepted this revision.Jan 12 2021, 9:34 AM
zoecarver added inline comments.
libcxx/include/atomic
1799

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.

Hi @ldionne,

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

__simt__ accepted this revision.Jan 19 2021, 9:36 AM

This change looks good to me, thanks!

ldionne requested changes to this revision.Jan 19 2021, 11:24 AM

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
1687–1688

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);
}
1798

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

1863

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
1 ↗(On Diff #316102)

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.

This revision now requires changes to proceed.Jan 19 2021, 11:24 AM
rarutyun added inline comments.Jan 25 2021, 2:44 PM
libcxx/include/atomic
1687–1688

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 updated this revision to Diff 319131.Jan 25 2021, 2:45 PM
rarutyun set the repository for this revision to rG LLVM Github Monorepo.
rarutyun marked an inline comment as done.
rarutyun added inline comments.
libcxx/include/atomic
1798

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?

1863

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 marked 2 inline comments as done.Jan 25 2021, 2:48 PM

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?

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
1687–1688

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.

1798

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 updated this revision to Diff 320372.Sun, Jan 31, 12:30 PM
rarutyun marked 4 inline comments as done.Sun, Jan 31, 12:35 PM

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
1798

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

rarutyun marked an inline comment as done.Sun, Jan 31, 12:35 PM
rarutyun updated this revision to Diff 320374.Sun, Jan 31, 1:04 PM
rarutyun updated this revision to Diff 320376.Sun, Jan 31, 1:54 PM
rarutyun updated this revision to Diff 320432.Mon, Feb 1, 4:06 AM
ldionne accepted this revision.Mon, Feb 1, 7:11 AM

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.

This revision is now accepted and ready to land.Mon, Feb 1, 7:11 AM
This revision was automatically updated to reflect the committed changes.