Allow _Atomic to be specified on incomplete types

[aaron.ballman]

Summary

The C standard does not prohibit the _Atomic specifier on incomplete types, which turns out to be sometimes useful.

This addresses PR37237.

Note that C++ still requires checking if the type is complete in order to support member pointers under the Microsoft ABI (because that check completes some information on the type), which is a use case we have in test\SemaCXX\atomic-type.cpp.

[efriedma]

This generally makes sense.

Need some tests to make sure we emit an appropriate error if you try to actually use atomic operators (load/store) or intrinsics (__atomic_is_lock_free etc.) with an incomplete type. And a test that code generation emits something appropriate.

lib/Sema/SemaType.cpp

8022

If you're going to allow incomplete types in C++, you'll need some code to handle the case where the type is initially incomplete, but completed later.

[rsmith]

lib/Sema/SemaType.cpp

8022

Perhaps the trivially-copyable requirement could be deferred in all cases until we reach an actual load or store. Constraints that are only enforced when a type happens to be complete are generally a bad idea (we have a mess like this for constraints on types being non-abstract, and we're still working on unsticking ourselves from that mess in the C++ standard).

In passing, I'd also note that we need the trivially-copyable requirement even outside C++, for structs containing __weak / __strong fields.

[aaron.ballman]

Hopefully address the review comments.

[aaron.ballman]

lib/Sema/SemaType.cpp

8022

I'm not quite certain I've properly captured the request here, but I took a stab at it. One thing to note is that our behavior for type checking __atomic_load() appears to diverge from GCC with regards to trivial copyability, even before my patch.

https://godbolt.org/g/g2fqLK

[efriedma]

I think the request was that we check that a type is trivially copyable when we perform an atomic operation? I don't see the code for that anywhere.

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

[aaron.ballman]

Addresses review comments.

[aaron.ballman]

Modifications based on review feedback.

[aaron.ballman]

In D46112#1091981, @efriedma wrote:

I think the request was that we check that a type is trivially copyable when we perform an atomic operation? I don't see the code for that anywhere.

Sorry about that -- I didn't notice that GNU was handled while C11 was not. That's been updated now.

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I believe the change I made to Type::isIncompleteType() is correct, but was surprised by the new behavior in SemaCXX/atomic-type.cpp that resulted. It appears that the RecordDecl for "struct inner" has IsCompleteDefinition set to false while instantiating struct atomic, but I'm not familiar enough with the template instantiation process to know whether this is reasonable or not.

[rsmith]

test/SemaCXX/atomic-type.cpp

5 ↗

(On Diff #146474)

This is a bug. You need to teach RequireCompleteType to look through atomic types when looking for a class type to instantiate.

[aaron.ballman]

Look through atomic types when checking type completeness during template instantiation.

[aaron.ballman]

test/SemaCXX/atomic-type.cpp

5 ↗

(On Diff #146474)

Thank you for the explanation -- I think I've addressed this in the latest patch.

[rsmith]

lib/Sema/SemaType.cpp

7604–7608

This function is duplicating the work of finding the type to complete, which was already done by isIncompleteType. Rather than unwrapping T here...

7636–7637

... these uses of T should be dyn_casting Def, which is the declaration that isIncompleteType said needed completing.

7648–7649

This would nicely address this FIXME, since isIncompleteType has already walked through arrays.

7679–7683

Likewise here, this should probably just be if (const auto *RD = dyn_cast<RecordDecl>(Def)).

[aaron.ballman]

Updated based on review comments.

[aaron.ballman]

lib/Sema/SemaType.cpp

7604–7608

Thank you for the explanation!

[rsmith]

In D46112#1096893, @aaron.ballman wrote:

In D46112#1091981, @efriedma wrote:

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I don't think this has sufficiently addressed the comment. Specifically, for a case like:

struct NotTriviallyCopyable { NotTriviallyCopyable(const NotTriviallyCopyable&); };
void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }

... we should reject the assignment, because it would perform an atomic operation on a non-trivially-copyable type. It would probably suffice to check the places where we form an AtomicToNonAtomic or NonAtomicToAtomic conversion.

In passing, I noticed that this crashes Clang (because we fail to create an lvalue-to-rvalue conversion for x):

struct X {};                              
void f(_Atomic X *p, X x) { *p = x; }

This will likely get in the way of your test cases unless you fix it :)

lib/Sema/SemaChecking.cpp

3202 ↗

(On Diff #146594)

The (pre-existing) isScalarType() check here looks wrong to me. If we had a non-trivially-copyable scalar type (which I think do not currently exist in our type system), I think we should reject it here.

lib/Sema/SemaType.cpp

7662

May as well cast directly to CXXRecordDecl here; none of the code controlled by this if does anything in C.

[aaron.ballman]

Corrected some of the review comments.

[aaron.ballman]

In D46112#1098243, @rsmith wrote:

In D46112#1096893, @aaron.ballman wrote:

In D46112#1091981, @efriedma wrote:

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I don't think this has sufficiently addressed the comment. Specifically, for a case like:

struct NotTriviallyCopyable { NotTriviallyCopyable(const NotTriviallyCopyable&); };
void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }

... we should reject the assignment, because it would perform an atomic operation on a non-trivially-copyable type. It would probably suffice to check the places where we form an AtomicToNonAtomic or NonAtomicToAtomic conversion.

In passing, I noticed that this crashes Clang (because we fail to create an lvalue-to-rvalue conversion for x):

struct X {};                              
void f(_Atomic X *p, X x) { *p = x; }

This will likely get in the way of your test cases unless you fix it :)

It only gets in the way for C++ whereas my primary concern for this patch is C. I tried spending a few hours on this today and got nowhere -- we have a lot of FIXME comments surrounding atomic type qualifiers in C++. I've run out of time to be able to work on this patch and may have to abandon it. I'd hate to lose the forward progress already made, so I'm wondering if the C bits are sufficiently baked that even more FIXMEs around atomics in C++ would suffice?

[efriedma]

If you don't want to spend too much time on C++, fine; could you add a short Objective-C test instead to make sure the trivially-copyable checks are working?

What are the changes to Sema::RequireCompleteTypeImpl supposed to do?

test/Sema/atomic-type.c

29

This error message is terrible; yes, technically 'void' isn't trivially copyable, but that isn't really helpful.

[jfb]

In D46112#1113557, @aaron.ballman wrote:

In D46112#1098243, @rsmith wrote:

In D46112#1096893, @aaron.ballman wrote:

In D46112#1091981, @efriedma wrote:

Also needs some test coverage for atomic operations which aren't calls, like "typedef struct S S; void f(_Atomic S *s, _Atomic S *s2) { *s = *s2; };".

Thank you for pointing this out -- that uncovered an issue where we were not properly diagnosing the types as being incomplete. I've added a new test case and rolled the contents of Sema\atomic-type.cpp (which I added in an earlier patch) into SemaCXX\atomic-type.cpp (which already existed and I missed it).

I don't think this has sufficiently addressed the comment. Specifically, for a case like:

struct NotTriviallyCopyable { NotTriviallyCopyable(const NotTriviallyCopyable&); };
void f(_Atomic NotTriviallyCopyable *p) { *p = *p; }

... we should reject the assignment, because it would perform an atomic operation on a non-trivially-copyable type. It would probably suffice to check the places where we form an AtomicToNonAtomic or NonAtomicToAtomic conversion.

In passing, I noticed that this crashes Clang (because we fail to create an lvalue-to-rvalue conversion for x):

struct X {};                              
void f(_Atomic X *p, X x) { *p = x; }

This will likely get in the way of your test cases unless you fix it :)

It only gets in the way for C++ whereas my primary concern for this patch is C. I tried spending a few hours on this today and got nowhere -- we have a lot of FIXME comments surrounding atomic type qualifiers in C++. I've run out of time to be able to work on this patch and may have to abandon it. I'd hate to lose the forward progress already made, so I'm wondering if the C bits are sufficiently baked that even more FIXMEs around atomics in C++ would suffice?

FYI I ran into this here: https://reviews.llvm.org/D49458

include/clang/Basic/DiagnosticSemaKinds.td

   "block pointer to non-function type is invalid">;
 def err_return_block_has_expr : Error<"void block should not return a value">;
 def err_block_return_missing_expr : Error<
   "non-void block should return a value">;
 def err_func_def_incomplete_result : Error<
   "incomplete result type %0 in function definition">;
 def err_atomic_specifier_bad_type : Error<
   "_Atomic cannot be applied to "
-  "%select{incomplete |array |function |reference |atomic |qualified |}0type "
+  "%select{array |function |reference |atomic |qualified |}0type "
   "%1 %select{||||||which is not trivially copyable}0">;
 
 // Expressions.
 def ext_sizeof_alignof_function_type : Extension<
   "invalid application of '%select{sizeof|alignof|vec_step}0' to a "
   "function type">, InGroup<PointerArith>;
 def ext_sizeof_alignof_void_type : Extension<
   "invalid application of '%select{sizeof|alignof|vec_step}0' to a void "

lib/Sema/SemaType.cpp

     if (Def &&
         !hasVisibleDefinition(Def, &SuggestedDef, /*OnlyNeedComplete*/true)) {
       // If the user is going to see an error here, recover by making the
       // definition visible.
       bool TreatAsComplete = Diagnoser && !isSFINAEContext();
       if (Diagnoser)
         diagnoseMissingImport(Loc, SuggestedDef, MissingImportKind::Definition,
                               /*Recover*/TreatAsComplete);
       return !TreatAsComplete;
     } else if (Def && !TemplateInstCallbacks.empty()) {
       CodeSynthesisContext TempInst;
       TempInst.Kind = CodeSynthesisContext::Memoization;
       TempInst.Template = Def;
       TempInst.Entity = Def;
       TempInst.PointOfInstantiation = Loc;
       atTemplateBegin(TemplateInstCallbacks, *this, TempInst);
       atTemplateEnd(TemplateInstCallbacks, *this, TempInst);
     }
 
     return false;
   }

   // could be completed by the module)?
   //
   // FIXME: Should we map through to the base array element type before
   // checking for a tag type?
   if (Tag || IFace) {
     NamedDecl *D =
         Tag ? static_cast<NamedDecl *>(Tag->getDecl()) : IFace->getDecl();
 
     // Avoid diagnosing invalid decls as incomplete.
     if (D->isInvalidDecl())
       return true;
 
     // Give the external AST source a chance to complete the type.
     if (auto *Source = Context.getExternalSource()) {
       if (Tag) {
         TagDecl *TagD = Tag->getDecl();
         if (TagD->hasExternalLexicalStorage())
           Source->CompleteType(TagD);
       } else {
         ObjCInterfaceDecl *IFaceD = IFace->getDecl();
         if (IFaceD->hasExternalLexicalStorage())
           Source->CompleteType(IFace->getDecl());
       }
       // If the external source completed the type, go through the motions
       // again to ensure we're allowed to use the completed type.
       if (!T->isIncompleteType())
         return RequireCompleteTypeImpl(Loc, T, Diagnoser);
     }
   }
 
   // If we have a class template specialization or a class member of a
   // class template specialization, or an array with known size of such,
   // try to instantiate it.
   QualType MaybeTemplate = T;
   while (const ConstantArrayType *Array
            = Context.getAsConstantArrayType(MaybeTemplate))
     MaybeTemplate = Array->getElementType();
   if (const RecordType *Record = MaybeTemplate->getAs<RecordType>()) {
     bool Instantiated = false;
     bool Diagnosed = false;
     if (ClassTemplateSpecializationDecl *ClassTemplateSpec
           = dyn_cast<ClassTemplateSpecializationDecl>(Record->getDecl())) {
       if (ClassTemplateSpec->getSpecializationKind() == TSK_Undeclared) {
         Diagnosed = InstantiateClassTemplateSpecialization(
             Loc, ClassTemplateSpec, TSK_ImplicitInstantiation,
             /*Complain=*/Diagnoser);
         Instantiated = true;
       }
     } else if (CXXRecordDecl *Rec
                  = dyn_cast<CXXRecordDecl>(Record->getDecl())) {
       CXXRecordDecl *Pattern = Rec->getInstantiatedFromMemberClass();
       if (!Rec->isBeingDefined() && Pattern) {
         MemberSpecializationInfo *MSI = Rec->getMemberSpecializationInfo();
         assert(MSI && "Missing member specialization information?");
         // This record was instantiated from a class within a template.
         if (MSI->getTemplateSpecializationKind() !=
             TSK_ExplicitSpecialization) {
           Diagnosed = InstantiateClass(Loc, Rec, Pattern,
                                        getTemplateInstantiationArgs(Rec),
                                        TSK_ImplicitInstantiation,
                                        /*Complain=*/Diagnoser);
           Instantiated = true;
         }
       }

                                         UnaryTransformType::EnumUnderlyingType);
     }
   }
   llvm_unreachable("unknown unary transform type");
 }
 
 QualType Sema::BuildAtomicType(QualType T, SourceLocation Loc) {
   if (!T->isDependentType()) {
-    // FIXME: It isn't entirely clear whether incomplete atomic types
-    // are allowed or not; for simplicity, ban them for the moment.
-    if (RequireCompleteType(Loc, T, diag::err_atomic_specifier_bad_type, 0))
-      return QualType();
-
     int DisallowedKind = -1;
     if (T->isArrayType())
-      DisallowedKind = 1;
+      DisallowedKind = 0;
     else if (T->isFunctionType())
-      DisallowedKind = 2;
+      DisallowedKind = 1;
     else if (T->isReferenceType())
-      DisallowedKind = 3;
+      DisallowedKind = 2;
     else if (T->isAtomicType())
-      DisallowedKind = 4;
+      DisallowedKind = 3;
     else if (T.hasQualifiers())
-      DisallowedKind = 5;
-    else if (!T.isTriviallyCopyableType(Context))
+      DisallowedKind = 4;
+    else if (LangOpts.CPlusPlus && isCompleteType(Loc, T) &&
+             !T.isTriviallyCopyableType(Context))
       // Some other non-trivially-copyable type (probably a C++ class)
-      DisallowedKind = 6;
+      DisallowedKind = 5;
 
     if (DisallowedKind != -1) {
       Diag(Loc, diag::err_atomic_specifier_bad_type) << DisallowedKind << T;
       return QualType();
     }
 
     // FIXME: Do we need any handling for ARC here?
   }
 
   // Build the pointer type.
   return Context.getAtomicType(T);
 }

test/Sema/atomic-type.c

void f(void) {
   _Atomic(_Atomic(int)*) *t4[2] = { &t3, 0 };
   testf(t4);
 }
 extern _Atomic(int (*)(int(*)[], int(*)[10])) mergetest;
 extern _Atomic(int (*)(int(*)[10], int(*)[])) mergetest;
 extern _Atomic(int (*)(int(*)[10], int(*)[10])) mergetest;
 
 _Atomic(int()) error1; // expected-error {{_Atomic cannot be applied to function type}}
-_Atomic(struct ErrorS) error2; // expected-error {{_Atomic cannot be applied to incomplete type}} expected-note {{forward declaration}}
+_Atomic(struct ErrorS) error2;
 _Atomic(int[10]) error3; // expected-error {{_Atomic cannot be applied to array type}}
 _Atomic(const int) error4; // expected-error {{_Atomic cannot be applied to qualified type}}
 _Atomic(_Atomic(int)) error5; // expected-error {{_Atomic cannot be applied to atomic type}}
+
+void g(_Atomic void *ptr);
+void h(_Atomic struct Incomplete *ptr); // expected-warning {{declaration of 'struct Incomplete' will not be visible outside of this function}}