r371222 - [Diagnostics] Refactor code for -Wsizeof-pointer-div, catch more cases; also add -Wsizeof-array-div
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 6 13:27:53 PDT 2019
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?
On Fri, 6 Sep 2019 at 12:38, Nico Weber via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> This reports things like
>
> error: expresion will return the incorrect number of elements in the
> array; the array element type is 'const char *', not 'char *'
>
> which doesn't look quite right...
>
> On Fri, Sep 6, 2019 at 12:11 PM David Bolvansky via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: xbolva00
>> Date: Fri Sep 6 09:12:48 2019
>> New Revision: 371222
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=371222&view=rev
>> Log:
>> [Diagnostics] Refactor code for -Wsizeof-pointer-div, catch more cases;
>> also add -Wsizeof-array-div
>>
>> Previously, -Wsizeof-pointer-div failed to catch:
>> const int *r;
>> sizeof(r) / sizeof(int);
>>
>> Now fixed.
>> Also introduced -Wsizeof-array-div which catches bugs like:
>> sizeof(r) / sizeof(short);
>>
>> (Array element type does not match type of sizeof operand).
>>
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>> cfe/trunk/lib/Sema/SemaExpr.cpp
>> cfe/trunk/test/Sema/div-sizeof-ptr.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=371222&r1=371221&r2=371222&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Sep 6
>> 09:12:48 2019
>> @@ -3391,6 +3391,10 @@ def note_pointer_declared_here : Note<
>> def warn_division_sizeof_ptr : Warning<
>> "'%0' will return the size of the pointer, not the array itself">,
>> InGroup<DiagGroup<"sizeof-pointer-div">>;
>> +def warn_division_sizeof_array : Warning<
>> + "expresion will return the incorrect number of elements in the array;
>> the array "
>> + "element type is %0, not %1">,
>>
>
This warning text doesn't seem right (what is "the incorrect number of
elements"?).
Perhaps "expression does not compute the number of elements in the array
%0; element type is %1, not %2"?
(Also you misspelled "expression".)
> + InGroup<DiagGroup<"sizeof-array-div">>;
>>
>> def note_function_warning_silence : Note<
>> "prefix with the address-of operator to silence this warning">;
>> @@ -8003,7 +8007,7 @@ def warn_array_index_precedes_bounds : W
>> def warn_array_index_exceeds_bounds : Warning<
>> "array index %0 is past the end of the array (which contains %1 "
>> "element%s2)">, InGroup<ArrayBounds>;
>> -def note_array_index_out_of_bounds : Note<
>> +def note_array_declared_here : Note<
>> "array %0 declared here">;
>>
>> def warn_printf_insufficient_data_args : Warning<
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=371222&r1=371221&r2=371222&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Sep 6 09:12:48 2019
>> @@ -13008,7 +13008,7 @@ void Sema::CheckArrayAccess(const Expr *
>>
>> if (ND)
>> DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,
>> - PDiag(diag::note_array_index_out_of_bounds)
>> + PDiag(diag::note_array_declared_here)
>> << ND->getDeclName());
>> }
>>
>>
>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=371222&r1=371221&r2=371222&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Fri Sep 6 09:12:48 2019
>> @@ -9135,7 +9135,7 @@ static void checkArithmeticNull(Sema &S,
>> << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
>> }
>>
>> -static void DiagnoseDivisionSizeofPointer(Sema &S, Expr *LHS, Expr *RHS,
>> +static void DiagnoseDivisionSizeofPointerOrArray(Sema &S, Expr *LHS,
>> Expr *RHS,
>> SourceLocation Loc) {
>> const auto *LUE = dyn_cast<UnaryExprOrTypeTraitExpr>(LHS);
>> const auto *RUE = dyn_cast<UnaryExprOrTypeTraitExpr>(RHS);
>> @@ -9154,16 +9154,29 @@ static void DiagnoseDivisionSizeofPointe
>> else
>> RHSTy = RUE->getArgumentExpr()->IgnoreParens()->getType();
>>
>> - if (!LHSTy->isPointerType() || RHSTy->isPointerType())
>> - return;
>> - if (LHSTy->getPointeeType().getCanonicalType() !=
>> RHSTy.getCanonicalType())
>> - return;
>> -
>> - S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS <<
>> LHS->getSourceRange();
>> - if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
>> - if (const ValueDecl *LHSArgDecl = DRE->getDecl())
>> - S.Diag(LHSArgDecl->getLocation(), diag::note_pointer_declared_here)
>> - << LHSArgDecl;
>> + if (LHSTy->isPointerType() && !RHSTy->isPointerType()) {
>> + if (LHSTy->getPointeeType().getCanonicalType().getUnqualifiedType()
>> !=
>> + RHSTy.getCanonicalType().getUnqualifiedType())
>>
>
Do not compare QualTypes like this. Use
S.Context.hasSameUnqualifiedType(LHSTy->getPointeeType(), RHSTy) instead.
> + return;
>> +
>> + S.Diag(Loc, diag::warn_division_sizeof_ptr) << LHS <<
>> LHS->getSourceRange();
>> + if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
>> + if (const ValueDecl *LHSArgDecl = DRE->getDecl())
>> + S.Diag(LHSArgDecl->getLocation(),
>> diag::note_pointer_declared_here)
>> + << LHSArgDecl;
>> + }
>> + } else if (isa<ArrayType>(LHSTy) && !RHSTy->isArrayType()) {
>>
>
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.)
> + QualType ArrayElemTy = cast<ArrayType>(LHSTy)->getElementType();
>> + if (isa<ArrayType>(ArrayElemTy) ||
>> + ArrayElemTy.getCanonicalType().getUnqualifiedType() ==
>> + RHSTy.getCanonicalType().getUnqualifiedType())
>>
>
The most appropriate check here is probably
S.Context.hasSimilarType(ArrayElemTy, RHSTy).
> + return;
>> + S.Diag(Loc, diag::warn_division_sizeof_array) << ArrayElemTy <<
>> RHSTy;
>>
>
This warning will have a bogus message if sizeof(RHSTy) ==
sizeof(ArrayElemTy). We should at least use different warning text in that
case.
> + if (const auto *DRE = dyn_cast<DeclRefExpr>(LHSArg)) {
>> + if (const ValueDecl *LHSArgDecl = DRE->getDecl())
>> + S.Diag(LHSArgDecl->getLocation(), diag::note_array_declared_here)
>> + << LHSArgDecl;
>> + }
>> }
>> }
>>
>> @@ -9200,7 +9213,7 @@ QualType Sema::CheckMultiplyDivideOperan
>> return InvalidOperands(Loc, LHS, RHS);
>> if (IsDiv) {
>> DiagnoseBadDivideOrRemainderValues(*this, LHS, RHS, Loc, IsDiv);
>> - DiagnoseDivisionSizeofPointer(*this, LHS.get(), RHS.get(), Loc);
>> + DiagnoseDivisionSizeofPointerOrArray(*this, LHS.get(), RHS.get(),
>> Loc);
>> }
>> return compType;
>> }
>>
>> Modified: cfe/trunk/test/Sema/div-sizeof-ptr.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/div-sizeof-ptr.cpp?rev=371222&r1=371221&r2=371222&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/test/Sema/div-sizeof-ptr.cpp (original)
>> +++ cfe/trunk/test/Sema/div-sizeof-ptr.cpp Fri Sep 6 09:12:48 2019
>> @@ -7,18 +7,20 @@ int f(Ty (&Array)[N]) {
>>
>> typedef int int32;
>>
>> -void test(int *p, int **q) { // expected-note 5 {{pointer 'p'
>> declared here}}
>> - int a1 = sizeof(p) / sizeof(*p); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> - int a2 = sizeof p / sizeof *p; // expected-warning {{'sizeof p'
>> will return the size of the pointer, not the array itself}}
>> - int a3 = sizeof(p) / sizeof(int); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> - int a4 = sizeof(p) / sizeof(p[0]); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> +void test(int *p, int **q) { // expected-note 5 {{pointer 'p'
>> declared here}}
>> + const int *r; // expected-note {{pointer 'r'
>> declared here}}
>> + int a1 = sizeof(p) / sizeof(*p); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> + int a2 = sizeof p / sizeof *p; // expected-warning {{'sizeof p'
>> will return the size of the pointer, not the array itself}}
>> + int a3 = sizeof(p) / sizeof(int); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> + int a4 = sizeof(p) / sizeof(p[0]); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> int a5 = sizeof(p) / sizeof(int32); // expected-warning {{'sizeof (p)'
>> will return the size of the pointer, not the array itself}}
>> + int a6 = sizeof(r) / sizeof(int); // expected-warning {{'sizeof (r)'
>> will return the size of the pointer, not the array itself}}
>>
>> int32 *d; // expected-note 2 {{pointer 'd'
>> declared here}}
>> - int a6 = sizeof(d) / sizeof(int32); // expected-warning {{'sizeof (d)'
>> will return the size of the pointer, not the array itself}}
>> - int a7 = sizeof(d) / sizeof(int); // expected-warning {{'sizeof (d)'
>> will return the size of the pointer, not the array itself}}
>> + int a7 = sizeof(d) / sizeof(int32); // expected-warning {{'sizeof (d)'
>> will return the size of the pointer, not the array itself}}
>> + int a8 = sizeof(d) / sizeof(int); // expected-warning {{'sizeof (d)'
>> will return the size of the pointer, not the array itself}}
>>
>> - int a8 = sizeof(*q) / sizeof(**q); // expected-warning {{'sizeof (*q)'
>> will return the size of the pointer, not the array itself}}
>> + int a9 = sizeof(*q) / sizeof(**q); // expected-warning {{'sizeof (*q)'
>> will return the size of the pointer, not the array itself}}
>>
>> // Should not warn
>> int b1 = sizeof(int *) / sizeof(int);
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190906/c3b4ffb3/attachment-0001.html>
More information about the cfe-commits
mailing list