Implement CWG496: Is a volatile-qualified type really a POD?

[K-ballo]

Summary

Implement CWG496, volatile qualified types are not trivially copyable, special copy/move member functions are not trivial for classes with volatile qualified non-static data members. Fixes PR21070. Breaks CodeGenCXX/no-opt-volatile-memcpy.cpp, which specifically tests assignment for a struct with a volatile member resulting in memcpy.

[rsmith]

Looks great, thanks!

test/CXX/drs/dr4xx.cpp

1187

dr496: 3.7, please.

test/SemaCXX/type-traits.cpp

1863–1865

Please commit this test update and the bugfix for the array case of QualType::isTriviallyCopyableType separately from the rest of the patch.

[K-ballo]

Any suggestions on what to do with the failing testcase (CodeGenCXX/no-opt-volatile-memcpy.cpp)? This test checks that the issued @llvm.memcpy has isvolatile=true, but after this patch memcpy is no longer issued as those operations are no longer considered trivial. This could suggest that memcpy with isvolatile=true should still be used in those cases (albeit the docs say "it is unwise to depend on it"), or that there is dead code out there looking for volatile members in trivial operations.

test/SemaCXX/type-traits.cpp

1863–1865

I don't have commit rights, but I will take this out and prepare a new revision.

[rsmith]

Some possible approaches:

• update the tests to check that we don't memcpy in this case
• change FieldMemcpyizer (in CGClass) to allow volatile fields and create a volatile memcpy call

The test was added for PR9027, which is just trying to make sure we emit a volatile load/store in this case, and either approach satisfies its requirements, but checking for the absence of a memcpy seems like a somewhat better approach to me.

[K-ballo]

Addressed review comments. Added test for the newly introduced diagnostic.

CodeGenCXX/no-opt-volatile-memcpy.cpp still fails. I tried checking for the absence of @llvm.memcpy, but the non-volatile array member of the struct is still memcpy-ed (good!). I have little to no experience with codegen tests, let alone llvm ir, I could use some help to adjust the offending test case.

include/clang/Basic/DiagnosticSemaKinds.td

 def note_nontrivial_default_arg : Note<"because it has a default argument">;
 def note_nontrivial_variadic : Note<"because it is a variadic function">;
 def note_nontrivial_subobject : Note<
   "because the function selected to %select{construct|copy|move|copy|move|"
   "destroy}2 %select{base class|field}0 of type %1 is not trivial">;
 def note_nontrivial_objc_ownership : Note<
   "because type %0 has a member with %select{no|no|__strong|__weak|"
   "__autoreleasing}1 ownership">;
+def note_nontrivial_has_volatile_member : Note<
+  "because field %0 has volatile qualified type">;
 
 def err_static_data_member_not_allowed_in_anon_struct : Error<
   "static data member %0 not allowed in anonymous struct">;
 def ext_static_data_member_in_union : ExtWarn<
   "static data member %0 in union is a C++11 extension">, InGroup<CXX11>;
 def warn_cxx98_compat_static_data_member_in_union : Warning<
   "static data member %0 in union is incompatible with C++98">,
   InGroup<CXX98Compat>, DefaultIgnore;

lib/AST/DeclCXX.cpp

if (T->isReferenceType()) {
         data().HasUninitializedReferenceMember = true;
 
       // C++0x [class]p7:
       //   A standard-layout class is a class that:
       //    -- has no non-static data members of type [...] reference,
       data().IsStandardLayout = false;
     }
 
+    // C++11 [class.copy]p12, C++11 [class.copy]p25:
+    //   A [copy/move constructor, or copy/move assignment operator for a
+    //   class X] is trivial [...] if:
+    //    -- class X has no non-static data members of volatile-qualified
+    //       type, [...]
+    if (T.isVolatileQualified())
+      data().HasTrivialSpecialMembers &=
+          ~(SMF_CopyConstructor | SMF_MoveConstructor | SMF_CopyAssignment |
+            SMF_MoveAssignment);
+
     // Record if this field is the first non-literal or volatile field or base.
     if (!T->isLiteralType(Context) || T.isVolatileQualified())
       data().HasNonLiteralTypeFieldsOrBases = true;
 
     if (Field->hasInClassInitializer() ||
         (Field->isAnonymousStructOrUnion() &&
          Field->getType()->getAsCXXRecordDecl()->hasInClassInitializer())) {
       data().HasInClassInitializer = true;

lib/AST/Type.cpp

bool QualType::isTrivialType(ASTContext &Context) const {
   }
   
   // No other types can match.
   return false;
 }
 
 bool QualType::isTriviallyCopyableType(ASTContext &Context) const {
   if ((*this)->isArrayType())
-    return Context.getBaseElementType(*this).isTrivialType(Context);
+    return Context.getBaseElementType(*this).isTriviallyCopyableType(Context);
 
   if (Context.getLangOpts().ObjCAutoRefCount) {
     switch (getObjCLifetime()) {
     case Qualifiers::OCL_ExplicitNone:
       return true;
       
     case Qualifiers::OCL_Strong:
     case Qualifiers::OCL_Weak:

lib/Sema/SemaDeclCXX.cpp

for (const auto *FI : RD->fields()) {
     //    -- no non-static data member of its class has a
     //       brace-or-equal-initializer
     if (CSM == Sema::CXXDefaultConstructor && FI->hasInClassInitializer()) {
       if (Diagnose)
         S.Diag(FI->getLocation(), diag::note_nontrivial_in_class_init) << FI;
       return false;
     }
 
+    // C++11 [class.copy]p12, C++11 [class.copy]p25:
+    //   A copy/move [constructor or assignment operator] for a class X is
+    //   trivial if
+    //    -- class X has no non-static data members of volatile-qualified
+    //       type, [...]
+    if ((CSM == Sema::CXXCopyConstructor || CSM == Sema::CXXCopyAssignment ||
+         CSM == Sema::CXXMoveConstructor || CSM == Sema::CXXMoveAssignment) &&
+        FieldType.isVolatileQualified()) {
+      if (Diagnose)
+        S.Diag(FI->getLocation(), diag::note_nontrivial_has_volatile_member)
+            << FI;
+      return false;
+    }
+
     // Objective C ARC 4.3.5:
     //   [...] nontrivally ownership-qualified types are [...] not trivially
     //   default constructible, copy constructible, move constructible, copy
     //   assignable, move assignable, or destructible [...]
     if (S.getLangOpts().ObjCAutoRefCount &&
         FieldType.hasNonTrivialObjCLifetime()) {
       if (Diagnose)
         S.Diag(FI->getLocation(), diag::note_nontrivial_objc_ownership)
bool Sema::SpecialMemberIsTrivial(CXXMethodDecl *MD, CXXSpecialMember CSM,
   //       thereof), the constructor selected to copy/move that member is
   //       trivial
   //
   // C++11 [class.copy]p12, C++11 [class.copy]p25:
   //   A [default constructor or destructor] is trivial if
   //    -- for all of the non-static data members of its class that are of class
   //       type (or array thereof), each such class has a trivial [default
   //       constructor or destructor]
+  //   A copy/move [constructor or assignment operator] for a class X is
+  //   trivial if
+  //    -- class X has no non-static data members of volatile-qualified
+  //       type, [...]
   if (!checkTrivialClassMembers(*this, RD, CSM, ConstArg, Diagnose))
     return false;
 
   // C++11 [class.dtor]p5:
   //   A destructor is trivial if [...]
   //    -- the destructor is not virtual
   if (CSM == CXXDestructor && MD->isVirtual()) {
     if (Diagnose)

test/CXX/drs/dr4xx.cpp

namespace dr495 { // dr495: 3.5
   struct S2 {
     template<typename U> operator U();
     operator int() { return T::error; }
   };
   S2<int> s2;
   long n2 = s2;
 }
 
-namespace dr496 { // dr496: no
+namespace dr496 { // dr496: yes
   struct A { int n; };
   struct B { volatile int n; };
   int check1[ __is_trivially_copyable(const int) ? 1 : -1];
   int check2[!__is_trivially_copyable(volatile int) ? 1 : -1];
   int check3[ __is_trivially_constructible(A, const A&) ? 1 : -1];
-  // FIXME: This is wrong.
-  int check4[ __is_trivially_constructible(B, const B&) ? 1 : -1];
+  int check4[!__is_trivially_constructible(B, const B&) ? 1 : -1];
   int check5[ __is_trivially_assignable(A, const A&) ? 1 : -1];
-  // FIXME: This is wrong.
-  int check6[ __is_trivially_assignable(B, const B&) ? 1 : -1];
+  int check6[!__is_trivially_assignable(B, const B&) ? 1 : -1];
 }
 
 namespace dr497 { // dr497: yes
   void before() {
     struct S {
       mutable int i;
     };
     const S cs; // expected-error {{default initialization}} expected-note {{add an explicit initializer}}

test/SemaCXX/type-traits.cpp

 };
 struct TrivialMoveButNotCopy { 
   TrivialMoveButNotCopy &operator=(TrivialMoveButNotCopy&&) = default;
   TrivialMoveButNotCopy &operator=(const TrivialMoveButNotCopy&);
 };
 struct NonTrivialDefault {
   NonTrivialDefault();
 };
+struct HasVolatileMember {
+  volatile int i;
+};
 
 struct HasDest { ~HasDest(); };
 class  HasPriv { int priv; };
 class  HasProt { protected: int prot; };
 struct HasRef { int i; int& ref; HasRef() : i(0), ref(i) {} };
 struct HasNonPOD { NonPOD np; };
 struct HasVirt { virtual void Virt() {}; };
 typedef NonPOD NonPODAr[10];
void trivial_checks()
   { int arr[T(__is_trivially_copyable(DerivesHasProt))]; }
   { int arr[T(__is_trivially_copyable(Vector))]; }
   { int arr[T(__is_trivially_copyable(VectorExt))]; }
   { int arr[T(__is_trivially_copyable(HasCons))]; }
   { int arr[T(__is_trivially_copyable(HasRef))]; }
   { int arr[T(__is_trivially_copyable(HasNonPOD))]; }
   { int arr[T(__is_trivially_copyable(DerivesHasCons))]; }
   { int arr[T(__is_trivially_copyable(DerivesHasRef))]; }
+  { int arr[T(__is_trivially_copyable(NonTrivialDefault))]; }
+  { int arr[T(__is_trivially_copyable(NonTrivialDefault[]))]; }
+  { int arr[T(__is_trivially_copyable(NonTrivialDefault[3]))]; }
 
   { int arr[F(__is_trivially_copyable(HasCopyAssign))]; }
   { int arr[F(__is_trivially_copyable(HasMoveAssign))]; }
   { int arr[F(__is_trivially_copyable(HasDest))]; }
   { int arr[F(__is_trivially_copyable(HasVirt))]; }
   { int arr[F(__is_trivially_copyable(DerivesHasCopyAssign))]; }
   { int arr[F(__is_trivially_copyable(DerivesHasMoveAssign))]; }
   { int arr[F(__is_trivially_copyable(DerivesHasDest))]; }
void trivial_checks()
   { int arr[F((__is_trivially_constructible(ExtDefaulted,
                                             ExtDefaulted &&)))]; }
 
   { int arr[T((__is_trivially_constructible(TriviallyConstructibleTemplate<int>)))]; }
   { int arr[F((__is_trivially_constructible(class_forward)))]; } // expected-error {{incomplete type 'class_forward' used in type trait expression}}
   { int arr[F((__is_trivially_constructible(class_forward[])))]; }
   { int arr[F((__is_trivially_constructible(void)))]; }
 
+  { int arr[T((__is_trivially_constructible(HasVolatileMember)))]; }
+  { int arr[F((__is_trivially_constructible(HasVolatileMember, 
+                                            const HasVolatileMember &)))]; }
+  { int arr[F((__is_trivially_constructible(HasVolatileMember, 
+                                            HasVolatileMember &&)))]; }
+
   { int arr[T((__is_trivially_assignable(int&, int)))]; }
   { int arr[T((__is_trivially_assignable(int&, int&)))]; }
   { int arr[T((__is_trivially_assignable(int&, int&&)))]; }
   { int arr[T((__is_trivially_assignable(int&, const int&)))]; }
   { int arr[T((__is_trivially_assignable(POD&, POD)))]; }
   { int arr[T((__is_trivially_assignable(POD&, POD&)))]; }
   { int arr[T((__is_trivially_assignable(POD&, POD&&)))]; }
   { int arr[T((__is_trivially_assignable(POD&, const POD&)))]; }
void trivial_checks()
   { int arr[T((__is_trivially_assignable(HasDefaultTrivialCopyAssign&,
                                          HasDefaultTrivialCopyAssign&)))]; }
   { int arr[T((__is_trivially_assignable(HasDefaultTrivialCopyAssign&,
                                        const HasDefaultTrivialCopyAssign&)))]; }
   { int arr[T((__is_trivially_assignable(TrivialMoveButNotCopy&,
                                          TrivialMoveButNotCopy)))]; }
   { int arr[T((__is_trivially_assignable(TrivialMoveButNotCopy&,
                                          TrivialMoveButNotCopy&&)))]; }
+
+  { int arr[F((__is_trivially_assignable(HasVolatileMember, 
+                                         const HasVolatileMember &)))]; }
+  { int arr[F((__is_trivially_assignable(HasVolatileMember, 
+                                         HasVolatileMember &&)))]; }
 }
 
 void constructible_checks() {
   { int arr[T(__is_constructible(HasNoThrowConstructorWithArgs))]; }
   { int arr[F(__is_nothrow_constructible(HasNoThrowConstructorWithArgs))]; } // MSVC doesn't look into default args and gets this wrong.
 
   { int arr[T(__is_constructible(HasNoThrowConstructorWithArgs, HasCons))]; }
   { int arr[T(__is_nothrow_constructible(HasNoThrowConstructorWithArgs, HasCons))]; }