This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Update atomic synopsis and tests.
ClosedPublic

Authored by Mordante on Jun 6 2021, 5:52 AM.

Details

Reviewers
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Commits
rGa76e698787e7: [libc++] Update atomic synopsis and tests.
Summary

While looking at LWG-2988 and P0558 it seems the issues were already
implemented, but the synopsis wasn't updated. Some of the tests didn't
validate the noexcept status. A few tests were missing completely:

  • atomic_wait_explicit
  • atomic_notify_one
  • atomic_notify_all

Mark P0558 as complete, didn't investigate which version of libc++ first
includes this. It seems the paper has been retroactively applied. I
couldn't find whether this is correct, but looking at cppreference it
seems intended.

Completes

  • LWG-2988 Clause 32 cleanup missed one typename
  • P0558 Resolving atomic<T> named base class inconsistencies

Diff Detail

Event Timeline

Mordante created this revision.Jun 6 2021, 5:52 AM
Mordante requested review of this revision.Jun 6 2021, 5:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2021, 5:52 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Jun 8 2021, 6:45 AM
ldionne added a subscriber: ldionne.

Can you confirm that the corresponding tests are already testing this issue? LGTM if that's the case, otherwise would you kindly add some?

This revision is now accepted and ready to land.Jun 8 2021, 6:45 AM

Ping, are we waiting on something to land this?

This kind of fell under the radar. I had a look and it seems the synopsis of the tests isn't updated yet. I also want to look whether this means P0558 is fully implemented. Since addition changes my conflict with D103983 I prefer to land that one first.

Mordante retitled this revision from [libc++][nfc] Update atomic synopsis. to [libc++] Update atomic synopsis and tests..Oct 10 2021, 4:29 AM
Mordante edited the summary of this revision. (Show Details)
Mordante updated this revision to Diff 378496.Oct 10 2021, 4:34 AM

Mark P0558 as complete, didn't investigate which version of libc++ first
includes this. It seems the paper has been retroactively applied. I
couldn't find whether this is correct, but looking at cppreference it
seems intended.

The other changes are:

  • Update outdated synopsis in the tests.
  • Where applicable added ASSERT_NOEXCEPT tests
  • Added a atomic_wait_explicit test (based on atomic_wait)
  • Added a atomic_notify_one test (based on atomic_wait)
  • Added a atomic_notify_all test (based on atomic_wait and notify_all)
Quuxplusone requested changes to this revision.Oct 10 2021, 8:59 AM
Quuxplusone added a subscriber: Quuxplusone.

Requesting changes, but basically this looks like an improvement and I don't actually care to see it again before it lands. :)

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_compare_exchange_weak_explicit.pass.cpp
11 ↗(On Diff #378496)

Pre-existing: this comment isn't right, and remains not-right after your PR changes the line numbers. I didn't look to find out which line number it should have said.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange.pass.cpp
11 ↗(On Diff #378496)

Ditto here.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_exchange_explicit.pass.cpp
11 ↗(On Diff #378496)

Ditto here. (Etc. etc.)

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_one.pass.cpp
51 ↗(On Diff #378496)

Arguably, it'd be better to keep using 2 and 4 for this second set of tests, just to guard against the remote possibility that something goes wrong when the stack frame reuses the same memory address for both a variables.

Also, it looks like you removed the test coverage of std::atomic_wait(&t, T(some-other-value)), which is expected to return immediately as a no-op.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_wait_explicit.pass.cpp
60 ↗(On Diff #378496)

Would it make sense to test any other memory orders? I assume there's nothing we can really do in a test to verify that the codegen uses the "right" memory order, though, so whatever.

This revision now requires changes to proceed.Oct 10 2021, 8:59 AM
Mordante added inline comments.Oct 10 2021, 10:44 AM
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_compare_exchange_weak_explicit.pass.cpp
11 ↗(On Diff #378496)

I'll remove this line and in the other places.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_one.pass.cpp
51 ↗(On Diff #378496)

The tests for atomic_wait and atomic_wait_explicit test the nop. This test is for
atomic_notify_one so here there's no need to test this nop. (This test is based on the atomic_wait.)

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_wait_explicit.pass.cpp
60 ↗(On Diff #378496)

It might make sense, but I noticed the other tests also only test std::memory_order_seq_cst. In that case we should look at that for all these tests not only this one. Since we have some ARM build bots it would be interesting to test this. (On x86 the CPU often has a stronger memory order than the requested memory order.)

But I see that out of scope for this patch.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_one.pass.cpp
51 ↗(On Diff #378496)

Ah, I get it. OK.
Might be a good idea to insert assert(a.load() == T(3)) before t.join(), just for paranoia. Cf. line 46 of atomic_notify_all.pass.cpp.

Mordante updated this revision to Diff 378689.Oct 11 2021, 8:48 AM
Mordante marked 6 inline comments as done.

Addresses review comments.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.wait/atomic_notify_one.pass.cpp
51 ↗(On Diff #378496)

It might be paranoia here, but the atomic_wait tests also don't do this test. I'll add them.

ldionne accepted this revision.Oct 11 2021, 12:09 PM

LGTM! Thanks for cleaning this up.

libcxx/include/atomic
270

We could perhaps drop the typename here for readability, since this is only in a comment. WDYT?

Mordante marked an inline comment as done.Oct 12 2021, 8:25 AM
Mordante added inline comments.
libcxx/include/atomic
270

I don't mind to remove them. It might be they're not even required in C++20, but too lazy too look for the details of the paper.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 12 2021, 8:28 AM
This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.