New patch with new diagnostic message. I couldn't come up with a better wording
so i'm using your suggestion. I don't know of a good way to silence the warning.
I removed the check for HasSideEffects previously but had to take back. I
noticed that the patch triggered some false positives without it.
//Anders
.......................................................................................................................
Anders Rönnholm Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
Mobile: +46 (0)70 912 42 54
E-mail: [email protected]
www.evidente.se
________________________________________
Från: Jordan Rose [[email protected]]
Skickat: den 31 januari 2014 18:50
Till: Anders Rönnholm
Cc: [email protected]; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
Sorry to have let this slip! This is looking good, but I had one more thought
about the diagnostic message. It says "may yield unexpected results", but
doesn't really explain what those unexpected results are. I was wondering if we
could work the type into the message for the operator case.
"operand of sizeof is a binary expression of type %0, which may not be intended"
I don't like that wording either, but at least this one makes people say "what?
why isn't it [the type I actually want]?". Also, should there be a way to
silence the warning?
What do you think?
Jordan
On Jan 23, 2014, at 6:40 , Anders Rönnholm
<[email protected]<mailto:[email protected]>> wrote:
Hi,
New one with comments handled.
________________________________________
Från: Jordan Rose [[email protected]<mailto:[email protected]>]
Skickat: den 20 december 2013 19:15
Till: Anders Rönnholm
Cc: [email protected]<mailto:[email protected]>; Daniel Marjamäki;
Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
On Dec 10, 2013, at 4:38 , Anders Rönnholm
<[email protected]<mailto:[email protected]><mailto:[email protected]>>
wrote:
Are you OK to commit this patch or do you see more issues?
I'm not sure if anyone else has ideological concerns. There's always a flag to
turn this off, I suppose.
+ if (S.isSFINAEContext())
+ return;
Code style: extra indent?
+ if(E->HasSideEffects(S.getASTContext()))
+ return;
sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't want to
warn here.
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if(FD && FD->isFunctionTemplateSpecialization())
+ return;
Code style: space after if. (Above too, actually.)
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
I don't think this is correct...the user is only trying to get ptrdiff_t if
both the LHS and RHS are pointer-ish.
+def warn_sizeof_bin_op : Warning<
+ "using sizeof() on an expression with an operator may yield unexpected
results">,
+ InGroup<SizeofOnExpression>;
+
+def warn_sizeof_sizeof : Warning<
+ "using sizeof() on sizeof() may yield unexpected results.">,
+ InGroup<SizeofOnExpression>;
+
sizeof doesn't actually require parens, so we shouldn't put the parens in the
diagnostics.
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 204465)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -3315,6 +3315,51 @@
<< ICE->getSubExpr()->getType();
}
+/// \brief Warns if sizeof expression has a binary operation or contains a
+/// sizeof.
+static void warnOnSizeofOnExpression(Sema &S, const Expr *E) {
+ if (S.isSFINAEContext())
+ return;
+
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if (FD && FD->isFunctionTemplateSpecialization())
+ return;
+
+ if (E->HasSideEffects(S.getASTContext()))
+ return;
+
+ if (const BinaryOperator *Binop =
+ dyn_cast<BinaryOperator>(E->IgnoreParens())) {
+ if (!Binop->isMultiplicativeOp() &&
+ !Binop->isAdditiveOp())
+ return;
+
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
+
+ // Don't warn if expression is based on macro.
+ if ((Binop->getLHS()->getExprLoc().isMacroID()) ||
+ (Binop->getRHS()->getExprLoc().isMacroID()))
+ return;
+
+ SourceRange DiagRange(Binop->getLHS()->getLocStart(),
+ Binop->getRHS()->getLocEnd());
+ S.Diag(Binop->getOperatorLoc(), diag::warn_sizeof_bin_op) << DiagRange <<
+ Binop->getType();
+ } else if (const UnaryExprOrTypeTraitExpr *UnaryExpr =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(E->IgnoreParens())) {
+ if (UnaryExpr->getKind() != UETT_SizeOf)
+ return;
+
+ SourceRange DiagRange(UnaryExpr->getLocStart(),
+ UnaryExpr->getLocEnd());
+ S.Diag(UnaryExpr->getOperatorLoc(), diag::warn_sizeof_sizeof) << DiagRange;
+ }
+}
+
/// \brief Check the constraints on expression operands to unary type expression
/// and type traits.
///
@@ -3377,6 +3422,7 @@
warnOnSizeofOnArrayDecay(*this, BO->getOperatorLoc(), BO->getType(),
BO->getRHS());
}
+ warnOnSizeofOnExpression(*this, E);
}
return false;
Index: test/Sema/exprs.c
===================================================================
--- test/Sema/exprs.c (revision 204465)
+++ test/Sema/exprs.c (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+// RUN: %clang_cc1 %s -Wno-sizeof-on-expression -verify -pedantic -fsyntax-only
// PR 8876 - don't warn about trivially unreachable null derefs. Note that
// we put this here because the reachability analysis only kicks in for
Index: test/Sema/sizeofonexpression.c
===================================================================
--- test/Sema/sizeofonexpression.c (revision 0)
+++ test/Sema/sizeofonexpression.c (revision 0)
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define DEF (1+2)
+
+struct s {
+ int xx;
+ int yy;
+};
+
+int SizeofStructExpression(struct s p) {
+ return sizeof(sizeof(p) + 1); // expected-warning{{operand of sizeof is a binary expression of type 'unsigned long', which may not be intended}}
+}
+
+int SizeofStructMemberExpression(struct s p) {
+ return sizeof(p.xx * p.yy); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+}
+
+int SizeofLiteralExpression() {
+ return sizeof(2 + 1); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+}
+
+int SizeofRefExpression() {
+ int x = 1, y = 2;
+ return sizeof(x / y); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+}
+
+int SizeofDefine() {
+ return sizeof(DEF);
+} // no-warning
+
+int SizeofDefineExpression() {
+ return sizeof(DEF + 2);
+} // no-warning
+
+int SizeofDefineExpression2() {
+ return sizeof(2 - DEF);
+} // no-warning
+
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1);
+} // no-warning
+
+int SizeofSizeof() {
+ return sizeof(sizeof(1)); // expected-warning{{using sizeof on sizeof may yield unexpected results}}
+}
+
+int SizeofComparison() {
+ return sizeof(1+1) == 1; // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+}
Index: test/SemaCXX/sizeofonexpression.cpp
===================================================================
--- test/SemaCXX/sizeofonexpression.cpp (revision 0)
+++ test/SemaCXX/sizeofonexpression.cpp (revision 0)
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+template<typename T, typename RESULT,
+ typename ARG1, typename ARG2, typename ARG3>
+class DoesDebug
+{
+ template <typename U, RESULT (U::*)(ARG1, ARG2, ARG3)> struct Check;
+ template <typename U> static char checkFn(Check<U, &U::Debug> *);
+ template <typename U> static int checkFn(...);
+public:
+ enum { Exist = sizeof(checkFn<T>(0)) == sizeof(char) }; // no-warning
+
+ int binOp() {
+ return sizeof(checkFn<T>(0)+1);
+ } // no-warning
+
+ int sizeofsizeof() {
+ return sizeof(sizeof(checkFn<T>(0)));
+ } // no-warning
+};
+class A {
+public:
+ A(): a(1) { }
+ A(int p) : a(sizeof(p + 1)) { } // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+ int a;
+};
+
+void f()
+{
+ int bb = 0;
+ int b = sizeof(bb + 1); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+}
Index: test/SemaCXX/decl-expr-ambiguity.cpp
===================================================================
--- test/SemaCXX/decl-expr-ambiguity.cpp (revision 204465)
+++ test/SemaCXX/decl-expr-ambiguity.cpp (working copy)
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors %s
-// RUN: %clang_cc1 -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors -x objective-c++ %s
+// RUN: %clang_cc1 -Wno-sizeof-on-expression -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors %s
+// RUN: %clang_cc1 -Wno-sizeof-on-expression -Wno-int-to-pointer-cast -fsyntax-only -verify -pedantic-errors -x objective-c++ %s
void f() {
int a;
Index: test/SemaTemplate/instantiate-var-template.cpp
===================================================================
--- test/SemaTemplate/instantiate-var-template.cpp (revision 204465)
+++ test/SemaTemplate/instantiate-var-template.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -std=c++1y %s
+// RUN: %clang_cc1 -Wno-sizeof-on-expression -verify -std=c++1y %s
namespace PR17846 {
template <typename T> constexpr T pi = T(3.14);
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 204465)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -4448,6 +4448,14 @@
"sizeof on pointer operation will return size of %0 instead of %1">,
InGroup<SizeofArrayDecay>;
+def warn_sizeof_bin_op : Warning<
+ "operand of sizeof is a binary expression of type %0, which may not be intended">,
+ InGroup<SizeofOnExpression>;
+
+def warn_sizeof_sizeof : Warning<
+ "using sizeof on sizeof may yield unexpected results">,
+ InGroup<SizeofOnExpression>;
+
def err_sizeof_nonfragile_interface : Error<
"application of '%select{alignof|sizeof}1' to interface %0 is "
"not supported on this architecture and platform">;
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td (revision 204465)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -278,6 +278,7 @@
def : DiagGroup<"synth">;
def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
+def SizeofOnExpression : DiagGroup<"sizeof-on-expression">;
def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
