Rework the warning for 'memset(p, 0, sizeof(p))' where 'p' is a pointer

and the programmer intended to write 'sizeof(*p)'. There are several
elements to the new version:

1) The actual expressions are compared in order to more accurately flag
   the case where the pattern that works for an array has been used, or
   a '*' has been omitted.
2) Only do a loose type-based check for record types. This prevents us
   from warning when we happen to be copying around chunks of data the
   size of a pointer and the pointer types for the sizeof and
   source/dest match.
3) Move all the diagnostics behind the runtime diagnostic filter. Not
   sure this is really important for this particular diagnostic, but
   almost everything else in SemaChecking.cpp does so.
4) Make the wording of the diagnostic more precise and informative. At
   least to my eyes.
5) Provide highlighting for the two expressions which had the unexpected
   similarity.
6) Place this diagnostic under a flag: -Wsizeof-pointer-memaccess

This uses the Stmt::Profile system for computing #1. Because of the
potential cost, this is guarded by the warning flag. I'd be interested
in feedback on how bad this is in practice; I would expect it to be
quite cheap in practice. Ideas for a cheaper / better way to do this are
also welcome.

The diagnostic wording could likely use some further wordsmithing.
Suggestions welcome here. The goals I had were to: clarify that its the
interaction of 'memset' and 'sizeof' and give more reasonable
suggestions for a resolution.

An open question is whether these diagnostics should have the note
attached for silencing by casting the dest/source pointer to void*.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@133155 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 12f5f30b3522d4be976ba06cd6858653f5510ab3..b608a5d694f3c61707056359842b25c5ea4e19f8 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -269,9 +269,15 @@ def warn_dyn_class_memaccess : Warning<
   InGroup<DiagGroup<"dynamic-class-memaccess">>;
 def note_bad_memaccess_silence : Note<
   "explicitly cast the pointer to silence this warning">;
-def warn_sizeof_pointer : Warning<
-  "the argument to sizeof is pointer type %0, expected %1 to match "
-  "%select{first|second}2 argument to %3">;
+def warn_sizeof_pointer_expr_memaccess : Warning<
+  "argument to 'sizeof' in %0 call is the same expression as the "
+  "%select{destination|source}1; did you mean to "
+  "%select{dereference it|remove the addressof|provide an explicit length}2?">,
+  InGroup<DiagGroup<"sizeof-pointer-memaccess">>;
+def warn_sizeof_pointer_type_memaccess : Warning<
+  "argument to 'sizeof' in %0 call is the same pointer type %1 as the "
+  "%select{destination|source}2; expected %3 or an explicit length">,
+  InGroup<DiagGroup<"sizeof-pointer-memaccess">>;
 
 /// main()
 // static/inline main() are not errors in C, just in C++.

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 945964fc351cb78545119c46f986b5ddd1759e77..81506bf571ab7e51fb6711c22d0b827914c85d87 100644 (file)
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -1828,18 +1828,25 @@ static bool isDynamicClassType(QualType T) {
   return false;
 }
 
-/// \brief If E is a sizeof expression, returns the expression's type in
-/// OutType.
-static bool sizeofExprType(const Expr* E, QualType *OutType) {
+/// \brief If E is a sizeof expression returns the argument expression,
+/// otherwise returns NULL.
+static const Expr *getSizeOfExprArg(const Expr* E) {
   if (const UnaryExprOrTypeTraitExpr *SizeOf =
-      dyn_cast<UnaryExprOrTypeTraitExpr>(E)) {
-    if (SizeOf->getKind() != clang::UETT_SizeOf)
-      return false;
+      dyn_cast<UnaryExprOrTypeTraitExpr>(E))
+    if (SizeOf->getKind() == clang::UETT_SizeOf && !SizeOf->isArgumentType())
+      return SizeOf->getArgumentExpr()->IgnoreParenImpCasts();
 
-    *OutType = SizeOf->getTypeOfArgument();
-    return true;
-  }
-  return false;
+  return 0;
+}
+
+/// \brief If E is a sizeof expression returns the argument type.
+static QualType getSizeOfArgType(const Expr* E) {
+  if (const UnaryExprOrTypeTraitExpr *SizeOf =
+      dyn_cast<UnaryExprOrTypeTraitExpr>(E))
+    if (SizeOf->getKind() == clang::UETT_SizeOf)
+      return SizeOf->getTypeOfArgument();
+
+  return QualType();
 }
 
 /// \brief Check for dangerous or invalid arguments to memset().
@@ -1858,6 +1865,12 @@ void Sema::CheckMemsetcpymoveArguments(const CallExpr *Call,
 
   unsigned LastArg = FnName->isStr("memset")? 1 : 2;
   const Expr *LenExpr = Call->getArg(2)->IgnoreParenImpCasts();
+
+  // We have special checking when the length is a sizeof expression.
+  QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
+  const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
+  llvm::FoldingSetNodeID SizeOfArgID;
+
   for (unsigned ArgIdx = 0; ArgIdx != LastArg; ++ArgIdx) {
     const Expr *Dest = Call->getArg(ArgIdx)->IgnoreParenImpCasts();
     SourceRange ArgRange = Call->getArg(ArgIdx)->getSourceRange();
@@ -1866,20 +1879,54 @@ void Sema::CheckMemsetcpymoveArguments(const CallExpr *Call,
     if (const PointerType *DestPtrTy = DestTy->getAs<PointerType>()) {
       QualType PointeeTy = DestPtrTy->getPointeeType();
 
-      // Don't warn about void pointers or char pointers as both are often used
-      // for directly representing memory, regardless of its underlying type.
-      if (PointeeTy->isVoidType() || PointeeTy->isCharType())
+      // Never warn about void type pointers. This can be used to suppress
+      // false positives.
+      if (PointeeTy->isVoidType())
         continue;
 
-      // Catch "memset(p, 0, sizeof(p))" -- needs to be sizeof(*p).
-      QualType SizeofTy;
-      if (sizeofExprType(LenExpr, &SizeofTy) &&
-          Context.typesAreCompatible(SizeofTy, DestTy)) {
-        // Note: This complains about sizeof(typeof(p)) as well.
-        SourceLocation loc = LenExpr->getSourceRange().getBegin();
-        Diag(loc, diag::warn_sizeof_pointer)
-            << SizeofTy <<  PointeeTy << ArgIdx << FnName;
-        break;
+      // Catch "memset(p, 0, sizeof(p))" -- needs to be sizeof(*p). Do this by
+      // actually comparing the expressions for equality. Because computing the
+      // expression IDs can be expensive, we only do this if the diagnostic is
+      // enabled.
+      if (SizeOfArg &&
+          Diags.getDiagnosticLevel(diag::warn_sizeof_pointer_expr_memaccess,
+                                   SizeOfArg->getExprLoc())) {
+        // We only compute IDs for expressions if the warning is enabled, and
+        // cache the sizeof arg's ID.
+        if (SizeOfArgID == llvm::FoldingSetNodeID())
+          SizeOfArg->Profile(SizeOfArgID, Context, true);
+        llvm::FoldingSetNodeID DestID;
+        Dest->Profile(DestID, Context, true);
+        if (DestID == SizeOfArgID) {
+          unsigned ActionIdx = 0; // Default is to suggest dereferencing.
+          if (const UnaryOperator *UnaryOp = dyn_cast<UnaryOperator>(Dest))
+            if (UnaryOp->getOpcode() == UO_AddrOf)
+              ActionIdx = 1; // If its an address-of operator, just remove it.
+          if (Context.getTypeSize(PointeeTy) == Context.getCharWidth())
+            ActionIdx = 2; // If the pointee's size is sizeof(char),
+                           // suggest an explicit length.
+          DiagRuntimeBehavior(SizeOfArg->getExprLoc(), Dest,
+                              PDiag(diag::warn_sizeof_pointer_expr_memaccess)
+                                << FnName << ArgIdx << ActionIdx
+                                << Dest->getSourceRange()
+                                << SizeOfArg->getSourceRange());
+          break;
+        }
+      }
+
+      // Also check for cases where the sizeof argument is the exact same
+      // type as the memory argument, and where it points to a user-defined
+      // record type.
+      if (SizeOfArgTy != QualType()) {
+        if (PointeeTy->isRecordType() &&
+            Context.typesAreCompatible(SizeOfArgTy, DestTy)) {
+          DiagRuntimeBehavior(LenExpr->getExprLoc(), Dest,
+                              PDiag(diag::warn_sizeof_pointer_type_memaccess)
+                                << FnName << SizeOfArgTy << ArgIdx
+                                << PointeeTy << Dest->getSourceRange()
+                                << LenExpr->getSourceRange());
+          break;
+        }
       }
 
       unsigned DiagID;

diff --git a/test/SemaCXX/warn-memset-bad-sizeof.cpp b/test/SemaCXX/warn-memset-bad-sizeof.cpp
index b334500ef5c63af5426c16c53c1291f214cd8a90..90ac50472e98df065f9a4563a5f32513962bb8df 100644 (file)
--- a/test/SemaCXX/warn-memset-bad-sizeof.cpp
+++ b/test/SemaCXX/warn-memset-bad-sizeof.cpp
@@ -30,23 +30,26 @@ void f(Mat m, const Foo& const_foo, char *buffer) {
   char arr[5];
   char* parr[5];
   Foo foo;
+  char* heap_buffer = new char[42];
 
   /* Should warn */
   memset(&s, 0, sizeof(&s));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memset'}}
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}}
   memset(ps, 0, sizeof(ps));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memset'}}
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}}
   memset(ps2, 0, sizeof(ps2));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'PS' (aka 'S *'), expected 'S' to match first argument to 'memset'}}
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}}
   memset(ps2, 0, sizeof(typeof(ps2)));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'typeof (ps2)' (aka 'S *'), expected 'S' to match first argument to 'memset'}}
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same pointer type}}
   memset(ps2, 0, sizeof(PS));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'PS' (aka 'S *'), expected 'S' to match first argument to 'memset'}}
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same pointer type}}
+  memset(heap_buffer, 0, sizeof(heap_buffer));  // \
+      // expected-warning {{argument to 'sizeof' in 'memset' call is the same expression as the destination}}
 
   memcpy(&s, 0, sizeof(&s));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match first argument to 'memcpy'}}
+      // expected-warning {{argument to 'sizeof' in 'memcpy' call is the same expression as the destination}}
   memcpy(0, &s, sizeof(&s));  // \
-      // expected-warning {{the argument to sizeof is pointer type 'S *', expected 'S' to match second argument to 'memcpy'}}
+      // expected-warning {{argument to 'sizeof' in 'memcpy' call is the same expression as the source}}
 
   /* Shouldn't warn */
   memset((void*)&s, 0, sizeof(&s));