Page MenuHomePhabricator

Disable use of _ExtInt with '__atomic' builtins
ClosedPublic

Authored by jtmott-intel on Jul 17 2020, 9:57 AM.

Details

Reviewers
erichkeane
jfb
Group Reviewers
Restricted Project
Commits
rGca77ab494aa2: Disable use of _ExtInt with '__atomic' builtins
Summary

We're (temporarily) disabling ExtInt for the '__atomic' builtins so we can better design their behavior later. The idea is until we do an audit/design for the way atomic builtins are supposed to work with _ExtInt, we should leave them restricted so they don't limit our future options, such as by binding us to a sub-optimal implementation via ABI.

Example after this change:

$ cat test.c

    void f(_ExtInt(64) *ptr) {
      __atomic_fetch_add(ptr, 1, 0);
    }

$ clang -c test.c

    test.c:2:22: error: argument to atomic builtin of type '_ExtInt' is not supported
      __atomic_fetch_add(ptr, 1, 0);
                         ^
    1 error generated.

Diff Detail

Event Timeline

jtmott-intel created this revision.Jul 17 2020, 9:57 AM

This change is intended to be merged into the release/11 branch.

erichkeane added inline comments.Jul 17 2020, 10:02 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
7970

I don't think this error message is proper. The 'please file a bug to request support' isn't something that we do.

Also, the odd 'ticks' are not something we use either. Hopefully someone can suggest better, but I'd say:

"argument to atomic builtin of type '_ExtInt' is not supported"

Updated diagnostic message. Unless someone suggests something different, I used Erich's message.

jtmott-intel marked an inline comment as done.Jul 17 2020, 10:21 AM
erichkeane accepted this revision.Jul 17 2020, 10:24 AM
This revision is now accepted and ready to land.Jul 17 2020, 10:24 AM
jtmott-intel edited the summary of this revision. (Show Details)Jul 17 2020, 12:16 PM

Hi @jfb . This commit it meant to address the concerns raised in https://reviews.llvm.org/D83340 . Thanks!

jfb added a comment.Jul 20 2020, 11:29 AM

I think you also want tests with the following:

#include <atomic>

_Atomic _ExtInt(8) ae0;
_Atomic(_ExtInt(8)) ae1;
std::atomic<_ExtInt(8)> ae3;
jfb added a comment.Jul 20 2020, 11:45 AM

Might be worth thinking about what volatile _ExtInt(1) ought to do as well :)

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

jfb added a subscriber: ldionne.Jul 29 2020, 8:22 AM

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

In D84049#2182011, @jfb wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

jfb added a subscriber: jwakely.Jul 29 2020, 9:28 AM
In D84049#2182011, @jfb wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

_Atomic and std::atomic ought to be compatible, they were designed to be. The original point I made was: "_ExtInt doesn't seem to be designed to work with atomic, please do something about it". It seems like disabling it for _Atomic is what you'd want at the moment, until a design for the entire thing is created?

We can ping @jwakely for libstdc++ 😃

In D84049#2182370, @jfb wrote:
In D84049#2182011, @jfb wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

_Atomic and std::atomic ought to be compatible, they were designed to be. The original point I made was: "_ExtInt doesn't seem to be designed to work with atomic, please do something about it". It seems like disabling it for _Atomic is what you'd want at the moment, until a design for the entire thing is created?

We can ping @jwakely for libstdc++ 😃

Ah, yes, a followup to disable this for the _Atomic types is probably a good idea. << @jtmott-intel
For context:
https://en.cppreference.com/w/c/language/atomic

It should be a lot like this diagnostic here: https://godbolt.org/z/K4Wrnf

jtmott-intel updated this revision to Diff 281742.EditedJul 29 2020, 2:49 PM

Modified _Atomic behavior to disallow *all* _ExtInt types of any size, which matches the behavior of __atomic builtins and std::atomic.

ldionne added a comment.EditedAug 4 2020, 6:51 AM

I think it makes sense to add a libc++ test (as libcxx/test/libcxx/atomics/ext-int.verify.cpp or similar). You can use the usual expected-error markup in those tests, they run under clang-verify.

But I'm not opposed to this patch getting in without the corresponding libc++ test (although it would be easy to add).

In D84049#2182370, @jfb wrote:

We can ping @jwakely for libstdc++ 😃

Whaddya want?

I don't even know what _ExtInt is. If Clang's __atomic_xxx built-ins give a diagnostic for _ExtInt types then libstdc++'s std::atomic shouldn't be able to work with them, because we just use the built-ins. Does Clang give the diagnostic even in system headers? That might be the only caveat.

Great! sounds like this might be resolved to everyone's satisfaction? I'll give this until end of Monday for other comments. cc @jfb

jfb accepted this revision.Aug 14 2020, 1:28 PM

@ldionne should sign off on the libcxx test, but the rest seems fine to me. Eventually we probably want to enable something, but need to be consistent about it.

This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 18 2020, 9:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Committed to master, and created bug to cherry pick into 11.0.

https://bugs.llvm.org/show_bug.cgi?id=47222

hans added a subscriber: hans.Aug 24 2020, 11:25 AM

Committed to master, and created bug to cherry pick into 11.0.

https://bugs.llvm.org/show_bug.cgi?id=47222

Pushed to 11.x as 82e48a579024d0ffbc352702ec0c52b47a6fe691. Thanks!

ldionne added inline comments.Sep 1 2020, 8:10 AM
libcxx/test/libcxx/atomics/ext-int.verify.cpp
1

This isn't great, since it won't run on clang-12, etc. I'll change it to:

// UNSUPPORTED: clang-4, clang-5, clang-6, clang-7, clang-8, clang-9, clang-10
// UNSUPPORTED: apple-clang-9, apple-clang-10, apple-clang-11
jtmott-intel added inline comments.Sep 1 2020, 8:20 AM
libcxx/test/libcxx/atomics/ext-int.verify.cpp
1

Thanks! That seems better.

ldionne added inline comments.Sep 1 2020, 9:03 AM
libcxx/test/libcxx/atomics/ext-int.verify.cpp
1