[cfe-commits] r133155 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/SemaCXX/warn-memset-bad-sizeof.cpp
Chandler Carruth
chandlerc at gmail.com
Thu Jun 16 02:09:41 PDT 2011
Author: chandlerc
Date: Thu Jun 16 04:09:40 2011
New Revision: 133155
URL: http://llvm.org/viewvc/llvm-project?rev=133155&view=rev
Log:
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*.
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=133155&r1=133154&r2=133155&view=diff
==============================================================================
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jun 16 04:09:40 2011
@@ -269,9 +269,15 @@
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++.
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=133155&r1=133154&r2=133155&view=diff
==============================================================================
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Jun 16 04:09:40 2011
@@ -1828,18 +1828,25 @@
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 @@
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 @@
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;
Modified: cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp?rev=133155&r1=133154&r2=133155&view=diff
==============================================================================
--- cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp (original)
+++ cfe/trunk/test/SemaCXX/warn-memset-bad-sizeof.cpp Thu Jun 16 04:09:40 2011
@@ -30,23 +30,26 @@
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));
More information about the cfe-commits
mailing list