[PATCH] A new warning, -Wtautological-array-compare, which warns when an array is compared against a null pointer
David Blaikie
dblaikie at gmail.com
Tue Jan 28 22:16:19 PST 2014
On Tue, Jan 28, 2014 at 10:01 PM, Richard Trieu <rtrieu at google.com> wrote:
> GCC's -Waddress doesn't catch this or (&x == 0). GCC's -Waddress does
> catch (x) for arrays, and (&x) for all types, but Clang doesn't.
>
Do you think your work would generalize/simplify to these cases? (even if
it does, depending on how much work that is, it might still be worth doing
this limited form you've done already - if it'd be easier to review it in
two steps (if the second step is a logical extension to the first) rather
than just reviewing the more general form)
> Clang's -Waddress is currently empty and only there for command line
> compatibility. Clang's warnings that overlap with GCC's -Waddress are
> spread out among other warning flags, for instance in some of the
> conversion warnings.
>
>
> On Tue, Jan 28, 2014 at 9:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> Does this overlap at all with GCC's -Waddress? (it might be that it
>> should be a subgroup of -Waddress - or generalized to catch some more cases
>> (what about &x == 0 or if (&x)))
>>
>>
>> On Tue, Jan 28, 2014 at 9:05 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>>> Extend -Wtautological-compare to warn when an array is compared to a
>>> null value. Arrays can be implicitly converted to a valid pointer, so it
>>> will never be null. Thus, the result of the comparison can be known at run
>>> time.
>>>
>>> This will not warn on externally linked weak arrays, which can be null,
>>> and inside macros, which can sometimes expect pointers. Several tests were
>>> updated to disable this warning.
>>>
>>> int foo() {
>>> int array[5];
>>> if (array != 0) // New warning here
>>> return array[0];
>>> else
>>> return 0;
>>> }
>>>
>>> http://llvm-reviews.chandlerc.com/D2645
>>>
>>> Files:
>>> test/Sema/const-eval.c
>>> test/SemaCXX/nullptr_in_arithmetic_ops.cpp
>>> test/SemaCXX/warn-tautological-compare.cpp
>>> test/SemaCXX/null_in_arithmetic_ops.cpp
>>> test/SemaCXX/constant-expression-cxx11.cpp
>>> include/clang/Basic/DiagnosticSemaKinds.td
>>> include/clang/Basic/DiagnosticGroups.td
>>> lib/Sema/SemaExpr.cpp
>>>
>>> Index: test/Sema/const-eval.c
>>> ===================================================================
>>> --- test/Sema/const-eval.c
>>> +++ test/Sema/const-eval.c
>>> @@ -1,4 +1,4 @@
>>> -// RUN: %clang_cc1 -fsyntax-only -verify -triple i686-linux %s
>>> +// RUN: %clang_cc1 -fsyntax-only -verify -triple i686-linux %s
>>> -Wno-tautological-array-compare
>>>
>>> #define EVAL_EXPR(testno, expr) int test##testno = sizeof(struct{char
>>> qq[expr];});
>>> int x;
>>> Index: test/SemaCXX/nullptr_in_arithmetic_ops.cpp
>>> ===================================================================
>>> --- test/SemaCXX/nullptr_in_arithmetic_ops.cpp
>>> +++ test/SemaCXX/nullptr_in_arithmetic_ops.cpp
>>> @@ -1,4 +1,4 @@
>>> -// RUN: %clang_cc1 -fsyntax-only -fblocks -std=c++11 -verify %s
>>> +// RUN: %clang_cc1 -fsyntax-only -Wno-tautological-array-compare
>>> -fblocks -std=c++11 -verify %s
>>>
>>> void foo() {
>>> int a;
>>> Index: test/SemaCXX/warn-tautological-compare.cpp
>>> ===================================================================
>>> --- test/SemaCXX/warn-tautological-compare.cpp
>>> +++ test/SemaCXX/warn-tautological-compare.cpp
>>> @@ -25,3 +25,40 @@
>>> if (x < kintmax) {}
>>> }
>>> }
>>> +
>>> +namespace ArrayCompare {
>>> +#define GetValue(ptr) ((ptr != 0) ? ptr[0] : 0)
>>> + extern int a[] __attribute__((weak));
>>> + int b[] = {8,13,21};
>>> + struct {
>>> + int x[10];
>>> + } c;
>>> + const char str[] = "text";
>>> + void ignore() {
>>> + if (a == 0) {}
>>> + if (a != 0) {}
>>> + (void)GetValue(b);
>>> + }
>>> + void test() {
>>> + if (b == 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [3]'
>>> equal to a null pointer is always false}}
>>> + if (b != 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [3]' not
>>> equal to a null pointer is always true}}
>>> + if (0 == b) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [3]'
>>> equal to a null pointer is always false}}
>>> + if (0 != b) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [3]' not
>>> equal to a null pointer is always true}}
>>> + if ("true" == 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'const char
>>> [5]' equal to a null pointer is always false}}
>>> + if ("true" != 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'const char
>>> [5]' not equal to a null pointer is always true}}
>>> + if (c.x != 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [10]' not
>>> equal to a null pointer is always true}}
>>> + if (c.x != 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'int [10]' not
>>> equal to a null pointer is always true}}
>>> + if (str == 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'const char
>>> [5]' equal to a null pointer is always false}}
>>> + if (str != 0) {}
>>> + // expected-warning at -1{{comparison of array of type 'const char
>>> [5]' not equal to a null pointer is always true}}
>>> + }
>>> +}
>>> Index: test/SemaCXX/null_in_arithmetic_ops.cpp
>>> ===================================================================
>>> --- test/SemaCXX/null_in_arithmetic_ops.cpp
>>> +++ test/SemaCXX/null_in_arithmetic_ops.cpp
>>> @@ -1,4 +1,4 @@
>>> -// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only
>>> -fblocks -Wnull-arithmetic -verify -Wno-string-plus-int %s
>>> +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only
>>> -fblocks -Wnull-arithmetic -verify -Wno-string-plus-int
>>> -Wno-tautological-array-compare %s
>>> #include <stddef.h>
>>>
>>> void f() {
>>> Index: test/SemaCXX/constant-expression-cxx11.cpp
>>> ===================================================================
>>> --- test/SemaCXX/constant-expression-cxx11.cpp
>>> +++ test/SemaCXX/constant-expression-cxx11.cpp
>>> @@ -1,4 +1,4 @@
>>> -// RUN: %clang_cc1 -triple i686-linux -Wno-string-plus-int
>>> -Wno-pointer-arith -Wno-zero-length-array -fsyntax-only -fcxx-exceptions
>>> -verify -std=c++11 -pedantic %s -Wno-comment
>>> +// RUN: %clang_cc1 -triple i686-linux -Wno-string-plus-int
>>> -Wno-pointer-arith -Wno-zero-length-array -fsyntax-only -fcxx-exceptions
>>> -verify -std=c++11 -pedantic %s -Wno-comment -Wno-tautological-array-compare
>>>
>>> namespace StaticAssertFoldTest {
>>>
>>> Index: include/clang/Basic/DiagnosticSemaKinds.td
>>> ===================================================================
>>> --- include/clang/Basic/DiagnosticSemaKinds.td
>>> +++ include/clang/Basic/DiagnosticSemaKinds.td
>>> @@ -4643,6 +4643,10 @@
>>> def warn_runsigned_always_true_comparison : Warning<
>>> "comparison of %0 unsigned%select{| enum}2 expression is always %1">,
>>> InGroup<TautologicalCompare>;
>>> +def warn_tautological_array_compare : Warning<
>>> + "comparison of array of type %0 %select{not equal to|equal to}1 a
>>> null "
>>> + "pointer is always %select{true|false}1">,
>>> + InGroup<TautologicalArrayCompare>;
>>> def warn_comparison_of_mixed_enum_types : Warning<
>>> "comparison of two values with different enumeration types"
>>> "%diff{ ($ and $)|}0,1">,
>>> Index: include/clang/Basic/DiagnosticGroups.td
>>> ===================================================================
>>> --- include/clang/Basic/DiagnosticGroups.td
>>> +++ include/clang/Basic/DiagnosticGroups.td
>>> @@ -283,8 +283,10 @@
>>> def StringPlusChar : DiagGroup<"string-plus-char">;
>>> def StrncatSize : DiagGroup<"strncat-size">;
>>> def TautologicalOutOfRangeCompare :
>>> DiagGroup<"tautological-constant-out-of-range-compare">;
>>> +def TautologicalArrayCompare : DiagGroup<"tautological-array-compare">;
>>> def TautologicalCompare : DiagGroup<"tautological-compare",
>>> - [TautologicalOutOfRangeCompare]>;
>>> + [TautologicalOutOfRangeCompare,
>>> + TautologicalArrayCompare]>;
>>> def HeaderHygiene : DiagGroup<"header-hygiene">;
>>> def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">;
>>> def CompareDistinctPointerType :
>>> DiagGroup<"compare-distinct-pointer-types">;
>>> Index: lib/Sema/SemaExpr.cpp
>>> ===================================================================
>>> --- lib/Sema/SemaExpr.cpp
>>> +++ lib/Sema/SemaExpr.cpp
>>> @@ -7646,6 +7646,30 @@
>>> }
>>> }
>>>
>>> +static void checkArrayNullCompare(Sema &S, SourceLocation Loc,
>>> + ExprResult &ArrayExpr, ExprResult
>>> &NullExpr,
>>> + bool IsEquality) {
>>> + if (Loc.isMacroID())
>>> + return;
>>> +
>>> + Expr *Array = ArrayExpr.get()->IgnoreParens();
>>> + ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(Array);
>>> + if (!ICE || ICE->getCastKind() != CK_ArrayToPointerDecay)
>>> + return;
>>> +
>>> + Array = ICE->getSubExpr();
>>> +
>>> + if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Array)) {
>>> + ValueDecl *VD = DRE->getDecl();
>>> + if (VD->hasExternalFormalLinkage() && VD->isWeak())
>>> + return;
>>> + }
>>> +
>>> + S.Diag(Loc, diag::warn_tautological_array_compare)
>>> + << Array->getSourceRange() << NullExpr.get()->getSourceRange()
>>> + << Array->getType() << IsEquality;
>>> +}
>>> +
>>> static void diagnoseLogicalNotOnLHSofComparison(Sema &S, ExprResult
>>> &LHS,
>>> ExprResult &RHS,
>>> SourceLocation Loc,
>>> @@ -7830,6 +7854,14 @@
>>> bool RHSIsNull = RHS.get()->isNullPointerConstant(Context,
>>>
>>> Expr::NPC_ValueDependentIsNull);
>>>
>>> + if (!IsRelational && LHSIsNull != RHSIsNull) {
>>> + bool IsEquality = Opc == BO_EQ;
>>> + if (RHSIsNull)
>>> + checkArrayNullCompare(*this, Loc, LHS, RHS, IsEquality);
>>> + else
>>> + checkArrayNullCompare(*this, Loc, RHS, LHS, IsEquality);
>>> + }
>>> +
>>> // All of the following pointer-related warnings are GCC extensions,
>>> except
>>> // when handling null pointer constants.
>>> if (LHSType->isPointerType() && RHSType->isPointerType()) { // C99
>>> 6.5.8p2
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140128/3b575e3b/attachment.html>
More information about the cfe-commits
mailing list