Page MenuHomePhabricator

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

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

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
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

It has been reported as PR47908. This is a proof-of-concept for
addressing the issue. With libc++ it's valid to use
atomic_fetch_add<int>(atomic<int*>*, atomic<int*>::difference_type);
MSVC and GCC reject this code: https://godbolt.org/z/9d8WzohbE

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

Note:

  • the patch only addresses issue in atomic_fetch_add
  • the modified constrains haven't been tested fully yet

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
2206–2207

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
2206–2207

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

Quuxplusone added inline comments.Jun 10 2021, 9:15 AM
libcxx/include/atomic
2221–2237

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
2221–2237

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.

Quuxplusone added inline comments.Jun 10 2021, 12:39 PM
libcxx/include/atomic
2221–2237

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
61–62

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
1851

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

Quuxplusone added inline comments.Jul 11 2021, 8:31 AM
libcxx/include/atomic
1851

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

.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
1851

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

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