Changed Checks from TriviallyCopyable to TriviallyCopyConstructible (#…

…77194) **Overview:** Fix a bug where Clang's range-loop-analysis incorrectly checks for trivial copyability instead of trivial copy constructibility, leading to erroneous warnings. Fixes #47355
llvm · Jan 10, 2024 · efcf192 · efcf192
1 parent 7fc7ef1
commit efcf192
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 10 deletions.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -884,6 +884,9 @@ Bug Fixes to AST Handling
 - Fixed a bug where RecursiveASTVisitor fails to visit the
   initializer of a bitfield.
   `Issue 64916 <https://github.com/llvm/llvm-project/issues/64916>`_
+- Fixed a bug where range-loop-analysis checks for trivial copyability,
+  rather than trivial copy-constructibility
+  `Issue 47355 <https://github.com/llvm/llvm-project/issues/47355>`_
 - Fixed a bug where Template Instantiation failed to handle Lambda Expressions
   with certain types of Attributes.
   (`#76521 <https://github.com/llvm/llvm-project/issues/76521>`_)

diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
@@ -1425,6 +1425,9 @@ class CXXRecordDecl : public RecordDecl {
   /// (C++11 [class]p6).
   bool isTriviallyCopyable() const;
 
+  /// Determine whether this class is considered trivially copyable per
+  bool isTriviallyCopyConstructible() const;
+
   /// Determine whether this class is considered trivial.
   ///
   /// C++11 [class]p6:

diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
@@ -917,6 +917,9 @@ class QualType {
   /// Return true if this is a trivially copyable type (C++0x [basic.types]p9)
   bool isTriviallyCopyableType(const ASTContext &Context) const;
 
+  /// Return true if this is a trivially copyable type
+  bool isTriviallyCopyConstructibleType(const ASTContext &Context) const;
+
   /// Return true if this is a trivially relocatable type.
   bool isTriviallyRelocatableType(const ASTContext &Context) const;
 

diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
@@ -587,6 +587,19 @@ bool CXXRecordDecl::isTriviallyCopyable() const {
   return true;
 }
 
+bool CXXRecordDecl::isTriviallyCopyConstructible() const {
+
+  //   A trivially copy constructible class is a class that:
+  //   -- has no non-trivial copy constructors,
+  if (hasNonTrivialCopyConstructor())
+    return false;
+  //   -- has a trivial destructor.
+  if (!hasTrivialDestructor())
+    return false;
+
+  return true;
+}
+
 void CXXRecordDecl::markedVirtualFunctionPure() {
   // C++ [class.abstract]p2:
   //   A class is abstract if it has at least one pure virtual function.

diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
@@ -2604,19 +2604,22 @@ bool QualType::isTrivialType(const ASTContext &Context) const {
   return false;
 }
 
-bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
-  if ((*this)->isArrayType())
-    return Context.getBaseElementType(*this).isTriviallyCopyableType(Context);
+static bool isTriviallyCopyableTypeImpl(const QualType &type,
+                                        const ASTContext &Context,
+                                        bool IsCopyConstructible) {
+  if (type->isArrayType())
+    return isTriviallyCopyableTypeImpl(Context.getBaseElementType(type),
+                                       Context, IsCopyConstructible);
 
-  if (hasNonTrivialObjCLifetime())
+  if (type.hasNonTrivialObjCLifetime())
     return false;
 
   // C++11 [basic.types]p9 - See Core 2094
   //   Scalar types, trivially copyable class types, arrays of such types, and
   //   cv-qualified versions of these types are collectively
-  //   called trivially copyable types.
+  //   called trivially copy constructible types.
 
-  QualType CanonicalType = getCanonicalType();
+  QualType CanonicalType = type.getCanonicalType();
   if (CanonicalType->isDependentType())
     return false;
 
@@ -2634,16 +2637,29 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
 
   if (const auto *RT = CanonicalType->getAs<RecordType>()) {
     if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
-      if (!ClassDecl->isTriviallyCopyable()) return false;
+      if (IsCopyConstructible) {
+        return ClassDecl->isTriviallyCopyConstructible();
+      } else {
+        return ClassDecl->isTriviallyCopyable();
+      }
     }
-
     return true;
   }
-
   // No other types can match.
   return false;
 }
 
+bool QualType::isTriviallyCopyableType(const ASTContext &Context) const {
+  return isTriviallyCopyableTypeImpl(*this, Context,
+                                     /*IsCopyConstructible=*/false);
+}
+
+bool QualType::isTriviallyCopyConstructibleType(
+    const ASTContext &Context) const {
+  return isTriviallyCopyableTypeImpl(*this, Context,
+                                     /*IsCopyConstructible=*/true);
+}
+
 bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
   QualType BaseElementType = Context.getBaseElementType(*this);
 

diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
@@ -3200,7 +3200,7 @@ static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef,
   // (The function `getTypeSize` returns the size in bits.)
   ASTContext &Ctx = SemaRef.Context;
   if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
-      (VariableType.isTriviallyCopyableType(Ctx) ||
+      (VariableType.isTriviallyCopyConstructibleType(Ctx) ||
        hasTrivialABIAttr(VariableType)))
     return;
 

diff --git a/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp b/clang/test/SemaCXX/warn-range-loop-analysis-trivially-copyable.cpp
@@ -33,6 +33,17 @@ void test_TriviallyCopyable_64_bytes() {
   for (const auto r : records)
     (void)r;
 }
+void test_TriviallyCopyConstructible_64_bytes() {
+  struct Record {
+    char a[64];
+    Record& operator=(Record const& other){return *this;};
+
+  };
+
+  Record records[8];
+  for (const auto r : records)
+    (void)r;
+}
 
 void test_TriviallyCopyable_65_bytes() {
   struct Record {
@@ -47,6 +58,19 @@ void test_TriviallyCopyable_65_bytes() {
     (void)r;
 }
 
+void test_TriviallyCopyConstructible_65_bytes() {
+  struct Record {
+    char a[65];
+    Record& operator=(Record const& other){return *this;};
+
+  };
+  // expected-warning@+3 {{loop variable 'r' creates a copy from type 'const Record'}}
+  // expected-note@+2 {{use reference type 'const Record &' to prevent copying}}
+  Record records[8];
+  for (const auto r : records)
+    (void)r;
+}
+
 void test_NonTriviallyCopyable() {
   struct Record {
     Record() {}
@@ -87,3 +111,4 @@ void test_TrivialABI_65_bytes() {
   for (const auto r : records)
     (void)r;
 }
+