Warn about memcpy'ing non-trivial C structs

[ahatanak]

Summary

Issue a warning when non-trivial C structs (structs with 'weak' or 'strong' fields) are copied or initialized by calls to memset, bzero, memcpy, and memmove. This is similar to gcc's -Wclass-memaccess warning. It might be better to add support for -Wclass-memaccess and make it cover both C and C++ structs that are non-trivial.

rdar://problem/36124208

[rjmccall]

include/clang/AST/NonTrivialCStructTypeVisitor.h

12 ↗

(On Diff #141108)

The header comment here was clearly just copied from another file.

I would name this header "NonTrivialTypeVisitor.h"; I don't think the CStruct adds anything, especially because the visitors actually start from types.

30 ↗

(On Diff #141108)

Should you have this pass the array type down? And is it really important to do this in the generic visitor? It seems like something you could do in an IRGen subclass.

include/clang/Basic/DiagnosticSemaKinds.td

620

I think this warning group should be -Wnontrivial-memaccess, and maybe -Wclass-memaccess should just be a subgroup of it.

lib/Sema/SemaChecking.cpp

7400

It would be better to just getAsArrayType, for generality purposes.

[ahatanak]

Address review comments.

include/clang/AST/NonTrivialCStructTypeVisitor.h

30 ↗

(On Diff #141108)

The subclasses in CGNonTrivialStruct.cpp need the size and the element type of the array to be passed to visitArray, so I think we have to pass the array type to visitArray. I guess it's possible to move this to the subclasses, but then the visit methods in the subclasses have to check whether the type is an array or not. I think we had a discussion on how arrays should be handled in this review: https://reviews.llvm.org/D41228.

But perhaps you have a better idea in mind?

include/clang/Basic/DiagnosticSemaKinds.td

620

Yes, we can create different DiagGroups when support for -Wclass-memaccess (warning for C++ classes) is added.

[rjmccall]

include/clang/AST/NonTrivialCStructTypeVisitor.h

30 ↗

(On Diff #141108)

Well, you could "override" the visit method in the subclass, e.g.:

template <class Derived, class RetTy = void>
class CGDestructedTypeVisitor : public DestructedTypeVisitor<Derived, RetTy> {
  using super = DestructedTypeVisitor<Derived, RetTy>;

public:
  using super::asDerived;
  using super::visit;

  template <class... Ts>
  RetTy visit(QualType::DestructionKind DK, QualType FT, Ts &&... Args) {
    if (asDerived().getContext().getAsArrayType(FT))
      return asDerived().visitArray(DK, FT, std::forward<Ts>(Args)...);

    return super::visit(DK, FT, std::forward<Ts>(Args)...);
  }
};

It's a bit more boilerplate, but I really feel like the array logic doesn't belong in the generic visitor.

About the array type: I wasn't trying to suggest that you should pass the element type to visitArray, I was suggesting you could just pass the array type as an ArrayType*, since that's what visitArray actually wants.

[ahatanak]

Move method visitArray to the derived classes and pass the array type to the method instead of the QualType.

include/clang/AST/NonTrivialCStructTypeVisitor.h

30 ↗

(On Diff #141108)

I incorporated both of your suggestions. I renamed the overloaded method 'visit' to 'visitWithKind' so that the 'visit' method that doesn't take the DestructionKind doesn't get hidden by the one that takes DestructionKind and is defined in the derived classes. There are a couple of places in CGNonTrivialStruct.cpp that call the former method.

[rjmccall]

lib/Sema/SemaChecking.cpp

7651

Indentation seems messed up in these two clauses.

[ahatanak]

Fix indentation.

[rjmccall]

Thanks; LGTM.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_invalid_cpu_is : Error<"invalid cpu name for builtin">;
 def err_builtin_needs_feature : Error<"%0 needs target feature %1">;
 def err_function_needs_feature
     : Error<"always_inline function %1 requires target feature '%2', but would "
             "be inlined into function %0 that is compiled without support for "
             "'%2'">;
 def warn_builtin_unknown : Warning<"use of unknown builtin %0">,
   InGroup<ImplicitFunctionDeclare>, DefaultError;
+def warn_cstruct_memaccess : Warning<
+  "%select{destination for|source of|first operand of|second operand of}0 this "
+  "%1 call is a pointer to record %2 that is not trivial to "
+  "%select{primitive-default-initialize|primitive-copy}3">,
+  InGroup<DiagGroup<"nontrivial-memaccess">>;
+def note_nontrivial_field : Note<
+  "field is non-trivial to %select{copy|default-initialize}0">;
 def warn_dyn_class_memaccess : Warning<
   "%select{destination for|source of|first operand of|second operand of}0 this "
   "%1 call is a pointer to %select{|class containing a }2dynamic class %3; "
   "vtable pointer will be %select{overwritten|copied|moved|compared}4">,
   InGroup<DiagGroup<"dynamic-class-memaccess">>;
 def note_bad_memaccess_silence : Note<
   "explicitly cast the pointer to silence this warning">;
 def warn_sizeof_pointer_expr_memaccess : Warning<

lib/Sema/SemaChecking.cpp

 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/NSAPI.h"
+#include "clang/AST/NonTrivialTypeVisitor.h"
 #include "clang/AST/OperationKinds.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Analysis/Analyses/FormatString.h"
 #include "clang/Basic/AddressSpaces.h"
static QualType getSizeOfArgType(const Expr *E) {
   if (const UnaryExprOrTypeTraitExpr *SizeOf =
       dyn_cast<UnaryExprOrTypeTraitExpr>(E))
     if (SizeOf->getKind() == UETT_SizeOf)
       return SizeOf->getTypeOfArgument();
 
   return QualType();
 }
 
+namespace {
+
+struct SearchNonTrivialToInitializeField
+    : DefaultInitializedTypeVisitor<SearchNonTrivialToInitializeField> {
+  using Super =
+      DefaultInitializedTypeVisitor<SearchNonTrivialToInitializeField>;
+
+  SearchNonTrivialToInitializeField(const Expr *E, Sema &S) : E(E), S(S) {}
+
+  void visitWithKind(QualType::PrimitiveDefaultInitializeKind PDIK, QualType FT,
+                     SourceLocation SL) {
+    if (const auto *AT = asDerived().getContext().getAsArrayType(FT)) {
+      asDerived().visitArray(PDIK, AT, SL);
+      return;
+    }
+
+    Super::visitWithKind(PDIK, FT, SL);
+  }
+
+  void visitARCStrong(QualType FT, SourceLocation SL) {
+    S.DiagRuntimeBehavior(SL, E, S.PDiag(diag::note_nontrivial_field) << 1);
+  }
+  void visitARCWeak(QualType FT, SourceLocation SL) {
+    S.DiagRuntimeBehavior(SL, E, S.PDiag(diag::note_nontrivial_field) << 1);
+  }
+  void visitStruct(QualType FT, SourceLocation SL) {
+    for (const FieldDecl *FD : FT->castAs<RecordType>()->getDecl()->fields())
+      visit(FD->getType(), FD->getLocation());
+  }
+  void visitArray(QualType::PrimitiveDefaultInitializeKind PDIK,
+                  const ArrayType *AT, SourceLocation SL) {
+    visit(getContext().getBaseElementType(AT), SL);
+  }
+  void visitTrivial(QualType FT, SourceLocation SL) {}
+
+  static void diag(QualType RT, const Expr *E, Sema &S) {
+    SearchNonTrivialToInitializeField(E, S).visitStruct(RT, SourceLocation());
+  }
+
+  ASTContext &getContext() { return S.getASTContext(); }
+
+  const Expr *E;
+  Sema &S;
+};
+
+struct SearchNonTrivialToCopyField
+    : CopiedTypeVisitor<SearchNonTrivialToCopyField, false> {
+  using Super = CopiedTypeVisitor<SearchNonTrivialToCopyField, false>;
+
+  SearchNonTrivialToCopyField(const Expr *E, Sema &S) : E(E), S(S) {}
+
+  void visitWithKind(QualType::PrimitiveCopyKind PCK, QualType FT,
+                     SourceLocation SL) {
+    if (const auto *AT = asDerived().getContext().getAsArrayType(FT)) {
+      asDerived().visitArray(PCK, AT, SL);
+      return;
+    }
+
+    Super::visitWithKind(PCK, FT, SL);
+  }
+
+  void visitARCStrong(QualType FT, SourceLocation SL) {
+    S.DiagRuntimeBehavior(SL, E, S.PDiag(diag::note_nontrivial_field) << 0);
+  }
+  void visitARCWeak(QualType FT, SourceLocation SL) {
+    S.DiagRuntimeBehavior(SL, E, S.PDiag(diag::note_nontrivial_field) << 0);
+  }
+  void visitStruct(QualType FT, SourceLocation SL) {
+    for (const FieldDecl *FD : FT->castAs<RecordType>()->getDecl()->fields())
+      visit(FD->getType(), FD->getLocation());
+  }
+  void visitArray(QualType::PrimitiveCopyKind PCK, const ArrayType *AT,
+                  SourceLocation SL) {
+    visit(getContext().getBaseElementType(AT), SL);
+  }
+  void preVisit(QualType::PrimitiveCopyKind PCK, QualType FT,
+                SourceLocation SL) {}
+  void visitTrivial(QualType FT, SourceLocation SL) {}
+  void visitVolatileTrivial(QualType FT, SourceLocation SL) {}
+
+  static void diag(QualType RT, const Expr *E, Sema &S) {
+    SearchNonTrivialToCopyField(E, S).visitStruct(RT, SourceLocation());
+  }
+
+  ASTContext &getContext() { return S.getASTContext(); }
+
+  const Expr *E;
+  Sema &S;
+};
+
+}
+
 /// \brief Check for dangerous or invalid arguments to memset().
 ///
 /// This issues warnings on known problematic, dangerous or unspecified
 /// arguments to the standard 'memset', 'memcpy', 'memmove', and 'memcmp'
 /// function calls.
 ///
 /// \param Call The call expression to diagnose.
 void Sema::CheckMemaccessArguments(const CallExpr *Call,
if (const CXXRecordDecl *ContainedRD =
           << Call->getCallee()->getSourceRange());
     } else if (PointeeTy.hasNonTrivialObjCLifetime() &&
              BId != Builtin::BImemset)
       DiagRuntimeBehavior(
         Dest->getExprLoc(), Dest,
         PDiag(diag::warn_arc_object_memaccess)
           << ArgIdx << FnName << PointeeTy
           << Call->getCallee()->getSourceRange());
-    else
+    else if (const auto *RT = PointeeTy->getAs<RecordType>()) {
+      if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
+          RT->getDecl()->isNonTrivialToPrimitiveDefaultInitialize()) {
+        DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
+                            PDiag(diag::warn_cstruct_memaccess)
+                                << ArgIdx << FnName << PointeeTy << 0);
+        SearchNonTrivialToInitializeField::diag(PointeeTy, Dest, *this);
+      } else if ((BId == Builtin::BImemcpy || BId == Builtin::BImemmove) &&
+                 RT->getDecl()->isNonTrivialToPrimitiveCopy()) {
+        DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
+                            PDiag(diag::warn_cstruct_memaccess)
+                                << ArgIdx << FnName << PointeeTy << 1);
+        SearchNonTrivialToCopyField::diag(PointeeTy, Dest, *this);
+      } else {
+        continue;
+      }
+    } else
       continue;
 
     DiagRuntimeBehavior(
       Dest->getExprLoc(), Dest,
       PDiag(diag::note_bad_memaccess_silence)
         << FixItHint::CreateInsertion(ArgRange.getBegin(), "(void*)"));
     break;
   }

test/SemaObjC/warn-nontrivial-struct-memaccess.m

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fobjc-runtime-has-weak -x objective-c -fobjc-arc -verify %s
+
+void *memset(void *, int, __SIZE_TYPE__);
+void bzero(void *, __SIZE_TYPE__);
+void *memcpy(void *, const void *, __SIZE_TYPE__);
+void *memmove(void *, const void *, __SIZE_TYPE__);
+
+struct Trivial {
+  int f0;
+  volatile int f1;
+};
+
+struct NonTrivial0 {
+  int f0;
+  __weak id f1; // expected-note 2 {{non-trivial to default-initialize}} expected-note 2 {{non-trivial to copy}}
+  volatile int f2;
+  id f3[10]; // expected-note 2 {{non-trivial to default-initialize}} expected-note 2 {{non-trivial to copy}}
+};
+
+struct NonTrivial1 {
+  id f0; // expected-note 2 {{non-trivial to default-initialize}} expected-note 2 {{non-trivial to copy}}
+  int f1;
+  struct NonTrivial0 f2;
+};
+
+void testTrivial(struct Trivial *d, struct Trivial *s) {
+  memset(d, 0, sizeof(struct Trivial));
+  bzero(d, sizeof(struct Trivial));
+  memcpy(d, s, sizeof(struct Trivial));
+  memmove(d, s, sizeof(struct Trivial));
+}
+
+void testNonTrivial1(struct NonTrivial1 *d, struct NonTrivial1 *s) {
+  memset(d, 0, sizeof(struct NonTrivial1)); // expected-warning {{that is not trivial to primitive-default-initialize}} expected-note {{explicitly cast the pointer to silence}}
+  memset((void *)d, 0, sizeof(struct NonTrivial1));
+  bzero(d, sizeof(struct NonTrivial1)); // expected-warning {{that is not trivial to primitive-default-initialize}} expected-note {{explicitly cast the pointer to silence}}
+  memcpy(d, s, sizeof(struct NonTrivial1)); // expected-warning {{that is not trivial to primitive-copy}} expected-note {{explicitly cast the pointer to silence}}
+  memmove(d, s, sizeof(struct NonTrivial1)); // expected-warning {{that is not trivial to primitive-copy}} expected-note {{explicitly cast the pointer to silence}}
+}