Page MenuHomePhabricator

Bring atomic header closer to C++20
ClosedPublic

Authored by __simt__ on Sep 8 2020, 12:42 PM.

Details

Reviewers
ldionne
jfb
Group Reviewers
Restricted Project
Summary

This patch:

  1. Adds missing feature test macros for atomic
  2. Updates the synopsis of atomic to match what's in the file
  3. Fills out the implemented features table
  4. Correctly uses the dependent types on atomic free functions
  5. Correctly uses memcpy in non-lock-free atomic CAS
  6. Adds missing tests for atomic_flag member functions
  7. Adds missing test cases for non-lock-free atomic objects

It seems to have also picked up some feature-test macro generated tests that were missing (?).

At present I have tested only on Linux but I'll test on Mac before submitting.

Diff Detail

Event Timeline

__simt__ created this revision.Sep 8 2020, 12:42 PM
Herald added a reviewer: jfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
__simt__ requested review of this revision.Sep 8 2020, 12:42 PM
__simt__ updated this revision to Diff 290568.Sep 8 2020, 12:53 PM

I figured out I could abandon most of the noisy changes. Dropping them out.

jfb added a subscriber: STL_MSFT.Sep 8 2020, 12:54 PM

Generally this looks good, but I didn't go into details.
It would be good to have an MS person like @STL_MSFT check that the std tests match, at least for what they've implemented.

libcxx/include/atomic
1255

Unrelated to your change, but the lock could come after __temp?

ldionne accepted this revision.Sep 8 2020, 1:01 PM

LGTM, but please remove whitespace changes in generate_feature_test_macro_components.py if I read the diff right. We can do those separately and ignore that git rev when blaming.

libcxx/utils/generate_feature_test_macro_components.py
90

You seem to have whitespace changes here? Or am I reading the Phabricator diff wrong?

This revision is now accepted and ready to land.Sep 8 2020, 1:01 PM
__simt__ updated this revision to Diff 290571.Sep 8 2020, 1:10 PM
__simt__ marked an inline comment as done.
__simt__ added inline comments.
libcxx/include/atomic
1255

Yep.

__simt__ marked an inline comment as done.Sep 8 2020, 1:12 PM
__simt__ added inline comments.
libcxx/utils/generate_feature_test_macro_components.py
90

Hmm. Fixing. Not sure why this is there.

__simt__ updated this revision to Diff 290573.Sep 8 2020, 1:16 PM

Removed spurious whitespace changes

__simt__ marked an inline comment as done.Sep 8 2020, 1:18 PM
__simt__ updated this revision to Diff 290591.Sep 8 2020, 3:10 PM

This version is now clean on Mac and Linux

@jfb

It would be good to have an MS person like @STL_MSFT check that the std tests match, at least for what they've implemented.

Unfortunately, we are currently unable to update our llvm-project submodule because a recent change to lit broke our test infrastructure - see https://github.com/microsoft/STL/pull/1090 . When we resolve that, we'll be able to run these tests against our atomic, but don't block on us. (If we find any issues we'll report back and/or submit a Phab PR and/or skip the affected tests in https://github.com/microsoft/STL/blob/master/tests/libcxx/skipped_tests.txt .)

@jfb

It would be good to have an MS person like @STL_MSFT check that the std tests match, at least for what they've implemented.

Unfortunately, we are currently unable to update our llvm-project submodule because a recent change to lit broke our test infrastructure - see https://github.com/microsoft/STL/pull/1090 . When we resolve that, we'll be able to run these tests against our atomic, but don't block on us. (If we find any issues we'll report back and/or submit a Phab PR and/or skip the affected tests in https://github.com/microsoft/STL/blob/master/tests/libcxx/skipped_tests.txt .)

Hmm, I wasn’t aware of that, but the breakage should have been expected. I’ll help you guys resolve the issue. The intent is to make it a lot easier to run the test suite by other vendors— actually to make such support first class.

ldionne added inline comments.Sep 9 2020, 4:56 AM
libcxx/test/std/atomics/types.pass.cpp
158

Can you please add a comment here saying #ifndef __APPLE__ // Apples doesn't ship libatomic so that we can at least enable that part of the test when/if we ship it?