[C23] Accept an _Atomic underlying type (#147802)

The underlying type of an enumeration is the non-atomic, unqualified
version of the specified type. Clang was rejecting such enumerations,
with a hard error, but now has the ability to downgrade the error into a
warning. Additionally, we diagnose (as a warning) dropping other
qualifiers. _Atomic is special given that an atomic type need not have
the same size as its non-atomic counterpart, and that the C++ version
of <stdatomic.h> defines _Atomic to std::atomic for easing cross-
language atomic use and std::atomic is an invalid enum base in C++.
(Note: we expose _Atomic in C++ even without including
<stdatomic,h>.)

Fixes #147736

Author: AaronBallman

clang/docs/ReleaseNotes.rst

@@ -312,6 +312,15 @@ C23 Feature Support
   `WG14 N2975 <https://open-std.org/JTC1/SC22/WG14/www/docs/n2975.pdf>`_
 - Fixed a bug with handling the type operand form of ``typeof`` when it is used
   to specify a fixed underlying type for an enumeration. #GH146351
+- Fixed a rejects-valid bug where Clang would reject an enumeration with an
+  ``_Atomic`` underlying type. The underlying type is the non-atomic,
+  unqualified version of the specified type. Due to the perhaps surprising lack
+  of atomic behavior, this is diagnosed under
+  ``-Wunderlying-atomic-qualifier-ignored``, which defaults to an error. This
+  can be downgraded with ``-Wno-underlying-atomic-qualifier-ignored`` or
+  ``-Wno-error=underlying-atomic-qualifier-ignored``. Clang now also diagnoses
+  cv-qualifiers as being ignored, but that warning does not default to an error.
+  It can be controlled by ``-Wunderlying-cv-qualifier-ignore``. (#GH147736)
 
 C11 Feature Support
 ^^^^^^^^^^^^^^^^^^^

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -9365,6 +9365,16 @@ def warn_atomic_implicit_seq_cst : Warning<
   InGroup<DiagGroup<"atomic-implicit-seq-cst">>, DefaultIgnore;
 def err_atomic_unsupported : Error<
   "atomic types are not supported in '%0'">;
+def warn_cv_stripped_in_enum : Warning<
+  "%enum_select<CVQualList>{"
+    "%Both{'const' and 'volatile' qualifiers}|"
+    "%Const{'const' qualifier}|"
+    "%Volatile{'volatile' qualifier}}0 in enumeration underlying type ignored">,
+  InGroup<DiagGroup<"underlying-cv-qualifier-ignored">>;
+def warn_atomic_stripped_in_enum : Warning<
+  "'_Atomic' qualifier ignored; operations involving the enumeration type will "
+  "be non-atomic">,
+  InGroup<DiagGroup<"underlying-atomic-qualifier-ignored">>, DefaultError;
 
 def err_overflow_builtin_must_be_int : Error<
   "operand argument to %select{overflow builtin|checked integer operation}0 "

clang/lib/Sema/SemaDecl.cpp

@@ -17155,6 +17155,30 @@ bool Sema::CheckEnumUnderlyingType(TypeSourceInfo *TI) {
   if (T->isDependentType())
     return false;
 
+  // C++0x 7.2p2: The type-specifier-seq of an enum-base shall name an
+  // integral type; any cv-qualification is ignored.
+  // C23 6.7.3.3p5: The underlying type of the enumeration is the unqualified,
+  // non-atomic version of the type specified by the type specifiers in the
+  // specifier qualifier list.
+  // Because of how odd C's rule is, we'll let the user know that operations
+  // involving the enumeration type will be non-atomic.
+  if (T->isAtomicType())
+    Diag(UnderlyingLoc, diag::warn_atomic_stripped_in_enum);
+
+  Qualifiers Q = T.getQualifiers();
+  std::optional<unsigned> QualSelect;
+  if (Q.hasConst() && Q.hasVolatile())
+    QualSelect = diag::CVQualList::Both;
+  else if (Q.hasConst())
+    QualSelect = diag::CVQualList::Const;
+  else if (Q.hasVolatile())
+    QualSelect = diag::CVQualList::Volatile;
+
+  if (QualSelect)
+    Diag(UnderlyingLoc, diag::warn_cv_stripped_in_enum) << *QualSelect;
+
+  T = T.getAtomicUnqualifiedType();
+
   // This doesn't use 'isIntegralType' despite the error message mentioning
   // integral type because isIntegralType would also allow enum types in C.
   if (const BuiltinType *BT = T->getAs<BuiltinType>())
@@ -17551,6 +17575,9 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
     } else if (UnderlyingType.get()) {
       // C++0x 7.2p2: The type-specifier-seq of an enum-base shall name an
       // integral type; any cv-qualification is ignored.
+      // C23 6.7.3.3p5: The underlying type of the enumeration is the
+      // unqualified, non-atomic version of the type specified by the type
+      // specifiers in the specifier qualifier list.
       TypeSourceInfo *TI = nullptr;
       GetTypeFromParser(UnderlyingType.get(), &TI);
       EnumUnderlying = TI;
@@ -17563,6 +17590,18 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
                                           UPPC_FixedUnderlyingType))
         EnumUnderlying = Context.IntTy.getTypePtr();
 
+      // If the underlying type is atomic, we need to adjust the type before
+      // continuing. This only happens in the case we stored a TypeSourceInfo
+      // into EnumUnderlying because the other cases are error recovery up to
+      // this point. But because it's not possible to gin up a TypeSourceInfo
+      // for a non-atomic type from an atomic one, we'll store into the Type
+      // field instead. FIXME: it would be nice to have an easy way to get a
+      // derived TypeSourceInfo which strips qualifiers including the weird
+      // ones like _Atomic where it forms a different type.
+      if (TypeSourceInfo *TI = dyn_cast<TypeSourceInfo *>(EnumUnderlying);
+          TI && TI->getType()->isAtomicType())
+        EnumUnderlying = TI->getType().getAtomicUnqualifiedType().getTypePtr();
+
     } else if (Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment()) {
       // For MSVC ABI compatibility, unfixed enums must use an underlying type
       // of 'int'. However, if this is an unfixed forward declaration, don't set

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

@@ -2022,8 +2022,17 @@ Decl *TemplateDeclInstantiator::VisitEnumDecl(EnumDecl *D) {
                                                 DeclarationName());
       if (!NewTI || SemaRef.CheckEnumUnderlyingType(NewTI))
         Enum->setIntegerType(SemaRef.Context.IntTy);
-      else
-        Enum->setIntegerTypeSourceInfo(NewTI);
+      else {
+        // If the underlying type is atomic, we need to adjust the type before
+        // continuing. See C23 6.7.3.3p5 and Sema::ActOnTag(). FIXME: same as
+        // within ActOnTag(), it would be nice to have an easy way to get a
+        // derived TypeSourceInfo which strips qualifiers including the weird
+        // ones like _Atomic where it forms a different type.
+        if (NewTI->getType()->isAtomicType())
+          Enum->setIntegerType(NewTI->getType().getAtomicUnqualifiedType());
+        else
+          Enum->setIntegerTypeSourceInfo(NewTI);
+      }
 
       // C++23 [conv.prom]p4
       // if integral promotion can be applied to its underlying type, a prvalue

clang/test/C/C23/n3030.c

@@ -91,3 +91,19 @@ enum e : short f = 0; // expected-error {{non-defining declaration of enumeratio
 enum g : short { yyy } h = yyy;
 
 enum ee2 : typeof ((enum ee3 : short { A })0, (short)0);
+
+enum not_actually_atomic : _Atomic(short) { // expected-error {{'_Atomic' qualifier ignored; operations involving the enumeration type will be non-atomic}}
+  Surprise
+};
+
+enum not_actually_const : const int { // expected-warning {{'const' qualifier in enumeration underlying type ignored}}
+  SurpriseAgain
+};
+
+enum not_actually_volatile : volatile int { // expected-warning {{'volatile' qualifier in enumeration underlying type ignored}}
+  SurpriseOnceMore
+};
+
+enum not_acually_const_or_volatile : const volatile int { // expected-warning {{'const' and 'volatile' qualifiers in enumeration underlying type ignored}}
+  WhyTheSurprise
+};

clang/test/C/C23/n3030_1.c

@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -std=c23 -Wno-underlying-atomic-qualifier-ignored -ast-dump %s | FileCheck %s
+
+// The underlying type is the unqualified, non-atomic version of the type
+// specified.
+enum const_enum : const short { ConstE };
+// CHECK: EnumDecl {{.*}} const_enum 'short'
+
+// These were previously being diagnosed as invalid underlying types. They
+// are valid; the _Atomic is stripped from the underlying type.
+enum atomic_enum1 : _Atomic(int) { AtomicE1 };
+// CHECK: EnumDecl {{.*}} atomic_enum1 'int'
+enum atomic_enum2 : _Atomic long long { AtomicE2 };
+// CHECK: EnumDecl {{.*}} atomic_enum2 'long long'

clang/test/CXX/dcl.dcl/dcl.enum/p2.cpp

@@ -1,6 +1,5 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
-// expected-no-diagnostics
-enum class E : int const volatile { };
+enum class E : int const volatile { }; // expected-warning {{'const' and 'volatile' qualifiers in enumeration underlying type ignored}}
 using T = __underlying_type(E);
 using T = int;

clang/test/CodeGen/enum3.c

@@ -0,0 +1,26 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-error=underlying-atomic-qualifier-ignored -std=c23 %s -emit-llvm -o - | FileCheck %s
+
+// Ensure that an "atomic" underlying type has no actual atomic semantics
+// because the qualifier is stripped.
+
+enum E : _Atomic(int) {
+  Foo
+};
+
+// CHECK-LABEL: define {{.*}} void @test(
+// CHECK-SAME: i32 noundef [[E:%.*]]) #[[ATTR0:[0-9]+]] {
+// CHECK-NEXT:  [[ENTRY:.*:]]
+// CHECK-NEXT:    [[E_ADDR:%.*]] = alloca i32
+// CHECK-NEXT:    [[X:%.*]] = alloca i32
+// CHECK-NEXT:    store i32 [[E]], ptr [[E_ADDR]]
+// CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[E_ADDR]]
+// CHECK-NEXT:    store i32 [[TMP0]], ptr [[X]]
+// CHECK-NEXT:    store i32 0, ptr [[E_ADDR]]
+// CHECK-NEXT:    ret void
+//
+void test(enum E e) {
+  int x = e;
+  e = Foo;
+}
+

clang/test/SemaCXX/enum-scoped.cpp

@@ -349,3 +349,18 @@ enum class B;
 A a;
 B b{a}; // expected-error {{cannot initialize}}
 }
+
+namespace GH147736 {
+template <typename Ty>
+struct S {
+  enum OhBoy : Ty { // expected-error 2 {{'_Atomic' qualifier ignored; operations involving the enumeration type will be non-atomic}}
+    Unimportant
+  } e;
+};
+
+// Okay, was previously rejected. The underlying type is int.
+S<_Atomic(int)> s; // expected-warning {{'_Atomic' is a C11 extension}}
+                   // expected-note@-1 {{in instantiation of template class 'GH147736::S<_Atomic(int)>' requested here}}
+static_assert(__is_same(__underlying_type(S<_Atomic(long long)>::OhBoy), long long), ""); // expected-warning {{'_Atomic' is a C11 extension}}
+                                                                                          // expected-note@-1 {{in instantiation of template class 'GH147736::S<_Atomic(long long)>' requested here}}
+}