<div dir="ltr"><div dir="ltr">In addition to the below, this also seems to be missing any test coverage for the new warning. Did you forget to add a test file?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 6 Sep 2019 at 12:38, Nico Weber via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">This reports things like<div><br></div><div>error: expresion will return the incorrect number of elements in the array; the array element type is 'const char *', not 'char *' <br></div><div><br></div><div>which doesn't look quite right...</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 6, 2019 at 12:11 PM David Bolvansky via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: xbolva00<br>
Date: Fri Sep 6 09:12:48 2019<br>
New Revision: 371222<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=371222&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=371222&view=rev</a><br>
Log:<br>
[Diagnostics] Refactor code for -Wsizeof-pointer-div, catch more cases; also add -Wsizeof-array-div<br>
<br>
Previously, -Wsizeof-pointer-div failed to catch:<br>
const int *r;<br>
sizeof(r) / sizeof(int);<br>
<br>
Now fixed.<br>
Also introduced -Wsizeof-array-div which catches bugs like:<br>
sizeof(r) / sizeof(short);<br>
<br>
(Array element type does not match type of sizeof operand).<br>
<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/lib/Sema/SemaChecking.cpp<br>
cfe/trunk/lib/Sema/SemaExpr.cpp<br>
cfe/trunk/test/Sema/div-sizeof-ptr.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=371222&r1=371221&r2=371222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=371222&r1=371221&r2=371222&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Sep 6 09:12:48 2019<br>
@@ -3391,6 +3391,10 @@ def note_pointer_declared_here : Note<<br>
def warn_division_sizeof_ptr : Warning<<br>
"'%0' will return the size of the pointer, not the array itself">,<br>
InGroup<DiagGroup<"sizeof-pointer-div">>;<br>
+def warn_division_sizeof_array : Warning<<br>
+ "expresion will return the incorrect number of elements in the array; the array "<br>
+ "element type is %0, not %1">,<br></blockquote></div></blockquote><div><br></div><div>This warning text doesn't seem right (what is "the incorrect number of elements"?).</div><div><br></div><div>Perhaps "expression does not compute the number of elements in the array %0; element type is %1, not %2"?</div><div><br></div><div>(Also you misspelled "expression".)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ InGroup<DiagGroup<"sizeof-array-div">>;<br>
<br>
def note_function_warning_silence : Note<<br>
"prefix with the address-of operator to silence this warning">;<br>
@@ -8003,7 +8007,7 @@ def warn_array_index_precedes_bounds : W<br>
def warn_array_index_exceeds_bounds : Warning<<br>
"array index %0 is past the end of the array (which contains %1 "<br>
"element%s2)">, InGroup<ArrayBounds>;<br>
-def note_array_index_out_of_bounds : Note<<br>
+def note_array_declared_here : Note<<br>
"array %0 declared here">;<br>
<br>
def warn_printf_insufficient_data_args : Warning<<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaChecking.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=371222&r1=371221&r2=371222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=371222&r1=371221&r2=371222&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Sep 6 09:12:48 2019<br>
@@ -13008,7 +13008,7 @@ void Sema::CheckArrayAccess(const Expr *<br>
<br>
if (ND)<br>
DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,<br>
- PDiag(diag::note_array_index_out_of_bounds)<br>
+ PDiag(diag::note_array_declared_here)<br>
<< ND->getDeclName());<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaExpr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=371222&r1=371221&r2=371222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=371222&r1=371221&r2=371222&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Sep 6 09:12:48 2019<br>
@@ -9135,7 +9135,7 @@ static void checkArithmeticNull(Sema &S,<br>
<< LHS.get()->getSourceRange() << RHS.get()->getSourceRange();<br>
}<br>
<br>
-static void DiagnoseDivisionSizeofPointer(Sema &S, Expr *LHS, Expr *RHS,<br>
+static void DiagnoseDivisionSizeofPointerOrArray(Sema &S, Expr *LHS, Expr *RHS,<br>
SourceLocation Loc) {<br>
const auto *LUE = dyn_cast<UnaryExprOrTypeTraitExpr>(LHS);<br>
const auto *RUE = dyn_cast<UnaryExprOrTypeTraitExpr>(RHS);<br>
@@ -9154,16 +9154,29 @@ static void DiagnoseDivisionSizeofPointe<br>
else<br>
RHSTy = RUE->getArgumentExpr()->IgnoreParens()->getType();<br>
<br>
- if (!LHSTy->isPointerType() || RHSTy->isPointerType())<br>
- return;<br>
- if (LHSTy->getPointeeType().getCanonicalType() != RHSTy.getCanonicalType())<br>
- return;<br>
-<br>
- S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS << LHS->getSourceRange();<br>
- if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {<br>
- if (const ValueDecl *LHSArgDecl = DRE->getDecl())<br>
- S.Diag(LHSArgDecl->getLocation(), diag::note_pointer_declared_here)<br>
- << LHSArgDecl;<br>
+ if (LHSTy->isPointerType() && !RHSTy->isPointerType()) {<br>
+ if (LHSTy->getPointeeType().getCanonicalType().getUnqualifiedType() !=<br>
+ RHSTy.getCanonicalType().getUnqualifiedType())<br></blockquote></div></blockquote><div><br></div><div>Do not compare QualTypes like this. Use S.Context.hasSameUnqualifiedType(LHSTy->getPointeeType(), RHSTy) instead.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return;<br>
+<br>
+ S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS << LHS->getSourceRange();<br>
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {<br>
+ if (const ValueDecl *LHSArgDecl = DRE->getDecl())<br>
+ S.Diag(LHSArgDecl->getLocation(), diag::note_pointer_declared_here)<br>
+ << LHSArgDecl;<br>
+ }<br>
+ } else if (isa<ArrayType>(LHSTy) && !RHSTy->isArrayType()) {<br></blockquote></div></blockquote><div><br></div><div>Why are you excluding the case where the RHS type is an array? That seems arbitrary. (Excluding the case where LHSTy and RHSTy are similar types seems like it might be reasonable, though; sizeof(x) / sizeof(x) should probably not be warned about here.)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ QualType ArrayElemTy = cast<ArrayType>(LHSTy)->getElementType();<br>
+ if (isa<ArrayType>(ArrayElemTy) ||<br>
+ ArrayElemTy.getCanonicalType().getUnqualifiedType() ==<br>
+ RHSTy.getCanonicalType().getUnqualifiedType())<br></blockquote></div></blockquote><div><br></div><div>The most appropriate check here is probably S.Context.hasSimilarType(ArrayElemTy, RHSTy).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ return;<br>
+ S.Diag(Loc, diag::warn_division_sizeof_array) << ArrayElemTy << RHSTy;<br></blockquote></div></blockquote><div><br></div><div>This warning will have a bogus message if sizeof(RHSTy) == sizeof(ArrayElemTy). We should at least use different warning text in that case.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {<br>
+ if (const ValueDecl *LHSArgDecl = DRE->getDecl())<br>
+ S.Diag(LHSArgDecl->getLocation(), diag::note_array_declared_here)<br>
+ << LHSArgDecl;<br>
+ }<br>
}<br>
}<br>
<br>
@@ -9200,7 +9213,7 @@ QualType Sema::CheckMultiplyDivideOperan<br>
return InvalidOperands(Loc, LHS, RHS);<br>
if (IsDiv) {<br>
DiagnoseBadDivideOrRemainderValues(*this, LHS, RHS, Loc, IsDiv);<br>
- DiagnoseDivisionSizeofPointer(*this, LHS.get(), RHS.get(), Loc);<br>
+ DiagnoseDivisionSizeofPointerOrArray(*this, LHS.get(), RHS.get(), Loc);<br>
}<br>
return compType;<br>
}<br>
<br>
Modified: cfe/trunk/test/Sema/div-sizeof-ptr.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/div-sizeof-ptr.cpp?rev=371222&r1=371221&r2=371222&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/div-sizeof-ptr.cpp?rev=371222&r1=371221&r2=371222&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Sema/div-sizeof-ptr.cpp (original)<br>
+++ cfe/trunk/test/Sema/div-sizeof-ptr.cpp Fri Sep 6 09:12:48 2019<br>
@@ -7,18 +7,20 @@ int f(Ty (&Array)[N]) {<br>
<br>
typedef int int32;<br>
<br>
-void test(int *p, int **q) { // expected-note 5 {{pointer 'p' declared here}}<br>
- int a1 = sizeof(p) / sizeof(*p); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
- int a2 = sizeof p / sizeof *p; // expected-warning {{'sizeof p' will return the size of the pointer, not the array itself}}<br>
- int a3 = sizeof(p) / sizeof(int); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
- int a4 = sizeof(p) / sizeof(p[0]); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
+void test(int *p, int **q) { // expected-note 5 {{pointer 'p' declared here}}<br>
+ const int *r; // expected-note {{pointer 'r' declared here}}<br>
+ int a1 = sizeof(p) / sizeof(*p); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
+ int a2 = sizeof p / sizeof *p; // expected-warning {{'sizeof p' will return the size of the pointer, not the array itself}}<br>
+ int a3 = sizeof(p) / sizeof(int); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
+ int a4 = sizeof(p) / sizeof(p[0]); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
int a5 = sizeof(p) / sizeof(int32); // expected-warning {{'sizeof (p)' will return the size of the pointer, not the array itself}}<br>
+ int a6 = sizeof(r) / sizeof(int); // expected-warning {{'sizeof (r)' will return the size of the pointer, not the array itself}}<br>
<br>
int32 *d; // expected-note 2 {{pointer 'd' declared here}}<br>
- int a6 = sizeof(d) / sizeof(int32); // expected-warning {{'sizeof (d)' will return the size of the pointer, not the array itself}}<br>
- int a7 = sizeof(d) / sizeof(int); // expected-warning {{'sizeof (d)' will return the size of the pointer, not the array itself}}<br>
+ int a7 = sizeof(d) / sizeof(int32); // expected-warning {{'sizeof (d)' will return the size of the pointer, not the array itself}}<br>
+ int a8 = sizeof(d) / sizeof(int); // expected-warning {{'sizeof (d)' will return the size of the pointer, not the array itself}}<br>
<br>
- int a8 = sizeof(*q) / sizeof(**q); // expected-warning {{'sizeof (*q)' will return the size of the pointer, not the array itself}}<br>
+ int a9 = sizeof(*q) / sizeof(**q); // expected-warning {{'sizeof (*q)' will return the size of the pointer, not the array itself}}<br>
<br>
// Should not warn<br>
int b1 = sizeof(int *) / sizeof(int);<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>