This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Improve atomic_fetch_(add|sub).*.
ClosedPublic

Authored by Mordante on Jun 9 2021, 11:41 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGaac5b84d4bf7: [libc++] Improve atomic_fetch_(add|sub).*.
Summary

While looking at the review comments in D103765 there was an oddity in
the tests for the following functions:

  • atomic_fetch_add
  • atomic_fetch_add_explicit
  • atomic_fetch_sub
  • atomic_fetch_sub_explicit

Libc++ allows usage of
atomic_fetch_add<int>(atomic<int*>*, atomic<int*>::difference_type);
MSVC and GCC reject this code: https://godbolt.org/z/9d8WzohbE

This makes the atomic fetch(add|sub).* Standard conforming and removes the non-conforming extensions.

Fixes PR47908

Diff Detail

Event Timeline

Mordante created this revision.Jun 9 2021, 11:41 AM
Mordante requested review of this revision.Jun 9 2021, 11:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 11:41 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/atomic
2172–2177

IMO, libc++ is wrong to constrain these functions at all.
https://eel.is/c++draft/atomics.nonmembers
does not mention any constraints. The only requirement on _Tp here is that typename atomic<_Tp>::difference_type must exist.

So, I would simply eliminate the enable_ifs here. Write

template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp atomic_fetch_add(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
    return __o->fetch_add(__op);
}

to match the Standard specification exactly.

Compare to atomic_store on line 1908, for example.

I'll look at implementing the proper constrains for the final patch.
For now I would like to determine whether we want to keep or remove the non-standard extension.
(Not sure whether they're extensions or bugs.)

libcxx/include/atomic
2172–2177

Agreed it would be better to add the constrains in the class.
This seems to be required already. In libc++ you can add to an atomic function pointer.
https://godbolt.org/z/effMvEr8j
This isn't allowed http://eel.is/c++draft/atomics.types.pointer#5

libcxx/include/atomic
2196–2212

This is not a compiler extension; I mean,

atomic<int*> a;
atomic_fetch_add(&a, 1);

must keep working no matter what. https://godbolt.org/z/ajdEYPhfK
I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
(I'm willing to commandeer, if what I'm saying isn't making sense to you.)

Mordante added inline comments.Jun 10 2021, 12:17 PM
libcxx/include/atomic
2196–2212

This is not a compiler extension; I mean,

atomic<int*> a;
atomic_fetch_add(&a, 1);

must keep working no matter what. https://godbolt.org/z/ajdEYPhfK

Correct, but that's not an extension. These two are https://godbolt.org/z/rs1oxYEMv

	atomic<int*> a;
	atomic_fetch_add<int>(&a, 1);

	atomic<int(*)()> f;
	atomic_fetch_add(&f, 1);

I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
(I'm willing to commandeer, if what I'm saying isn't making sense to you.)

What you say makes sense, but I just want to know whether we want to keep these 2 extensions (bugs?).
There's no need to commandeer, I just want to decide which direction to take before implementing the changes and the tests.

libcxx/include/atomic
2196–2212

Ah. Yeah, "obviously" it's OK to remove those usages.

  • Using atomic_fetch_add<int> with an explicit template argument violates the general SD-8-esque rules about explicit template arguments, e.g. make_pair<int, int>.
  • Incrementing a function pointer doesn't make sense semantically; currently we codegen it to a no-op; it was never supposed to be supported.

Lines 2196-2212 were clearly just trying to implement atomic_fetch_add for pointers (after accidentally/bogusly failing to support pointers in the primary template, due to that bogus constraint that needs removing).

Just to record this publicly:

The question is do we want to keep the current non-conforming code or do we want to remove it?

Remove them. Almost every time we've implemented an extension, it has ended up biting us. It just keeps biting us time after time. This is not a blanket statement to remove all extensions, but in this case, let's aim to be as close to the spec as possible, period.

Thanks for looking at this.

Mordante added inline comments.Jun 17 2021, 10:29 AM
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_add.pass.cpp
56

Use an array as @Quuxplusone suggested in D103769.

Mordante updated this revision to Diff 357800.Jul 11 2021, 7:29 AM

Rebased.
Remove the invalid overloads.
Validates the requirement the pointer points to a complete object type.
Use array's in the existing unit tests to avoid UB, as suggested by @Quuxplusone.
Test whether the functions are noexcept, as a small drive-by improvement.
Document the changes in behavior in the release notes.
Updated the patch to fix the issue for all add and sub functions.

Mordante added inline comments.Jul 11 2021, 7:32 AM
libcxx/include/atomic
1825

I'm not too happy with this way to constrain the function. I'm open to better suggestions.

libcxx/include/atomic
1825

I recommend:

_LIBCPP_INLINE_VISIBILITY
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT
    {return __cxx_atomic_fetch_add(&this->__a_, __op, __m);}

This follows the Standard's specification exactly, and also is simple (no weird metaprogramming involved). Also, this is what libstdc++ does. (MSVC essentially handles this via a partial specialization: they conditional_t between _Atomic_pointer and _Atomic_storage depending on whether the pointer is an object pointer type. But we don't need to do that, the standard doesn't do that, and libstdc++ doesn't do that. Let's Keep It Simple.)

If you want to explicitly nod to the "Mandates:" in http://eel.is/c++draft/atomics.types.pointer#5 , then I recommend

_LIBCPP_INLINE_VISIBILITY
_Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT {
    static_assert(is_object<typename remove_pointer<_Tp>::type>::value, "Must be an object pointer type");
    return __cxx_atomic_fetch_add(&this->__a_, __op, __m);
}
libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp
26 ↗(On Diff #357800)

.verify.cpp tests are run only on libc++? This test wouldn't pass against libstdc++, for example, because libstdc++ doesn't SFINAE away fetch_sub in these cases. (There'd still be an error; it'd just be a different error, because subtracting from a void* is ill-formed.)

Mordante marked an inline comment as done.Jul 12 2021, 1:03 PM

Thanks for the review!

libcxx/include/atomic
1825

I'll have a look at the other two implementations. However we need to do something else we do arithmetic on a function pointer. Or do you mean that Clang should protect us against that behavior? When I don't disable the function pointer here it allows function pointers.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp
26 ↗(On Diff #357800)

Correct these tests are only executed with Clang and libc++. So here it's valid to validate against Clang specific diagnostics.

Mordante retitled this revision from [libc++][rfc] Improve atomic_fetch_(add|sub).*. to [libc++] Improve atomic_fetch_(add|sub).*..Jul 25 2021, 7:38 AM
Mordante marked 5 inline comments as done.Oct 7 2021, 11:03 AM
Mordante added inline comments.
libcxx/include/atomic
1825

I did some testing. Clang accepts pointers to functions, so we need an static_assert to guard against these. The other invalid pointers generate a compiler diagnostic.

libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp
26 ↗(On Diff #357800)

I recently learned these still should be in the libcxx instead of the std part of the tests.

Mordante updated this revision to Diff 377918.Oct 7 2021, 11:15 AM

Addresses review comments.

Mordante edited the summary of this revision. (Show Details)Oct 7 2021, 11:21 AM
ldionne accepted this revision.Oct 7 2021, 11:41 AM
ldionne added inline comments.
libcxx/docs/ReleaseNotes.rst
74 ↗(On Diff #377918)
This revision is now accepted and ready to land.Oct 7 2021, 11:41 AM

Other than the troublesome release note, this LGTM (AFAIrecall and from a quick refresher).

libcxx/docs/ReleaseNotes.rst
62–63 ↗(On Diff #377918)

Here and on line 68, I strongly recommend removing the sentence This isn't allowed by the Standard.. I mean, we wouldn't make the change if it weren't allowed, right?
I think what you meant is The old behavior wasn't conforming; the new behavior is (more) conforming. But we don't need to say that either; those who trust their library vendor will assume that the change moves in the right direction (not the wrong direction), and those who don't trust, can just go read the Standard themselves if they want some kind of rationale for why we're suddenly implementing the Standard behavior.

This revision was automatically updated to reflect the committed changes.
Mordante marked 2 inline comments as done.
Mordante added inline comments.Oct 8 2021, 8:45 AM
libcxx/docs/ReleaseNotes.rst
62–63 ↗(On Diff #377918)

I don't feel it's wrong to mention it. However I'm not strongly attached to the sentence, so I've removed it.