This is a very useful warning, thanks. One issue I have with it is that it's disabled by Wno-tautological-compare. >From the warning name that makes sense – but Wtautological-compare used to be a fairly harmless warning, so we (chromium) disabled it for a bunch of third-party libraries. When Wtautological-undefined-compare arrived, we fixed all instances of this in our code and updated our compiler to a clang that optimizes away comparisons of references with NULL (since we had fixed all of these comparisons, we thought!)
I recently realized that we didn't see Wtautological-undefined-compare warnings for all the third-party libraries that we're building with Wno-tautological-compare. Do you think Wtautological-undefined-compare should be in its own warning group (and maybe have a different name to make that clear), so that folks who disabled Wtautological-compare still get this warning? The reasoning is that this warning is much more serious that what Wtautological-compare used to warn about. Nico On Fri, Jun 6, 2014 at 2:39 PM, Richard Trieu <[email protected]> wrote: > Author: rtrieu > Date: Fri Jun 6 16:39:26 2014 > New Revision: 210372 > > URL: http://llvm.org/viewvc/llvm-project?rev=210372&view=rev > Log: > Add -Wtautological-undefined-compare and -Wundefined-bool-conversion > warnings > to detect underfined behavior involving pointers. > > Added: > cfe/trunk/test/SemaCXX/warn-tautological-undefined-compare.cpp > cfe/trunk/test/SemaCXX/warn-undefined-bool-conversion.cpp > Modified: > cfe/trunk/include/clang/Basic/DiagnosticGroups.td > cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > cfe/trunk/lib/Sema/SemaChecking.cpp > cfe/trunk/test/Analysis/inlining/path-notes.cpp > cfe/trunk/test/Analysis/misc-ps-region-store.cpp > cfe/trunk/test/Analysis/reference.cpp > cfe/trunk/test/Analysis/stack-addr-ps.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Fri Jun 6 16:39:26 > 2014 > @@ -38,7 +38,9 @@ def LiteralConversion : DiagGroup<"liter > def StringConversion : DiagGroup<"string-conversion">; > def SignConversion : DiagGroup<"sign-conversion">; > def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">; > -def BoolConversion : DiagGroup<"bool-conversion", [ PointerBoolConversion > ] >; > +def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">; > +def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion, > + > UndefinedBoolConversion]>; > def IntConversion : DiagGroup<"int-conversion">; > def EnumConversion : DiagGroup<"enum-conversion">; > def FloatConversion : DiagGroup<"float-conversion">; > @@ -298,10 +300,12 @@ def StrncatSize : DiagGroup<"strncat-siz > def TautologicalOutOfRangeCompare : > DiagGroup<"tautological-constant-out-of-range-compare">; > def TautologicalPointerCompare : > DiagGroup<"tautological-pointer-compare">; > def TautologicalOverlapCompare : > DiagGroup<"tautological-overlap-compare">; > +def TautologicalUndefinedCompare : > DiagGroup<"tautological-undefined-compare">; > def TautologicalCompare : DiagGroup<"tautological-compare", > [TautologicalOutOfRangeCompare, > TautologicalPointerCompare, > - TautologicalOverlapCompare]>; > + TautologicalOverlapCompare, > + TautologicalUndefinedCompare]>; > def HeaderHygiene : DiagGroup<"header-hygiene">; > def DuplicateDeclSpecifier : DiagGroup<"duplicate-decl-specifier">; > def CompareDistinctPointerType : > DiagGroup<"compare-distinct-pointer-types">; > > Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Jun 6 > 16:39:26 2014 > @@ -2400,10 +2400,27 @@ def warn_impcast_pointer_to_bool : Warni > "address of%select{| function| array}0 '%1' will always evaluate to " > "'true'">, > InGroup<PointerBoolConversion>; > +def warn_this_bool_conversion : Warning< > + "'this' pointer cannot be null in well-defined C++ code; pointer may be > " > + "assumed always converted to true">, InGroup<UndefinedBoolConversion>; > +def warn_address_of_reference_bool_conversion : Warning< > + "reference cannot be bound to dereferenced null pointer in well-defined > C++ " > + "code; pointer may be assumed always converted to true">, > + InGroup<UndefinedBoolConversion>; > + > def warn_null_pointer_compare : Warning< > "comparison of %select{address of|function|array}0 '%1' %select{not > |}2" > "equal to a null pointer is always %select{true|false}2">, > InGroup<TautologicalPointerCompare>; > +def warn_this_null_compare : Warning< > + "'this' pointer cannot be null in well-defined C++ code; comparison may > be " > + "assumed to always evaluate to %select{true|false}0">, > + InGroup<TautologicalUndefinedCompare>; > +def warn_address_of_reference_null_compare : Warning< > + "reference cannot be bound to dereferenced null pointer in well-defined > C++ " > + "code; comparison may be assumed to always evaluate to " > + "%select{true|false}0">, > + InGroup<TautologicalUndefinedCompare>; > > def note_function_warning_silence : Note< > "prefix with the address-of operator to silence this warning">; > > Modified: cfe/trunk/lib/Sema/SemaChecking.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) > +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jun 6 16:39:26 2014 > @@ -6189,6 +6189,13 @@ void Sema::DiagnoseAlwaysNonNullPointer( > > const bool IsCompare = NullKind != Expr::NPCK_NotNull; > > + if (isa<CXXThisExpr>(E)) { > + unsigned DiagID = IsCompare ? diag::warn_this_null_compare > + : diag::warn_this_bool_conversion; > + Diag(E->getExprLoc(), DiagID) << E->getSourceRange() << Range << > IsEqual; > + return; > + } > + > bool IsAddressOf = false; > > if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) { > @@ -6218,9 +6225,14 @@ void Sema::DiagnoseAlwaysNonNullPointer( > // Address of function is used to silence the function warning. > if (IsFunction) > return; > - // Address of reference can be null. > - if (T->isReferenceType()) > + > + if (T->isReferenceType()) { > + unsigned DiagID = IsCompare > + ? diag::warn_address_of_reference_null_compare > + : > diag::warn_address_of_reference_bool_conversion; > + Diag(E->getExprLoc(), DiagID) << E->getSourceRange() << Range << > IsEqual; > return; > + } > } > > // Found nothing. > > Modified: cfe/trunk/test/Analysis/inlining/path-notes.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/path-notes.cpp?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/test/Analysis/inlining/path-notes.cpp (original) > +++ cfe/trunk/test/Analysis/inlining/path-notes.cpp Fri Jun 6 16:39:26 > 2014 > @@ -1,5 +1,5 @@ > -// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text > -analyzer-config c++-inlining=destructors -std=c++11 -verify %s > -// RUN: %clang_cc1 -analyze -analyzer-checker=core > -analyzer-output=plist-multi-file -analyzer-config c++-inlining=destructors > -std=c++11 -analyzer-config path-diagnostics-alternate=false %s -o %t.plist > +// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-output=text > -analyzer-config c++-inlining=destructors -std=c++11 -verify > -Wno-tautological-undefined-compare %s > +// RUN: %clang_cc1 -analyze -analyzer-checker=core > -analyzer-output=plist-multi-file -analyzer-config c++-inlining=destructors > -std=c++11 -analyzer-config path-diagnostics-alternate=false %s -o %t.plist > -Wno-tautological-undefined-compare > // RUN: FileCheck --input-file=%t.plist %s > > class Foo { > > Modified: cfe/trunk/test/Analysis/misc-ps-region-store.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/misc-ps-region-store.cpp?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/test/Analysis/misc-ps-region-store.cpp (original) > +++ cfe/trunk/test/Analysis/misc-ps-region-store.cpp Fri Jun 6 16:39:26 > 2014 > @@ -1,5 +1,5 @@ > -// RUN: %clang_cc1 -triple i386-apple-darwin9 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks > %s -fexceptions -fcxx-exceptions > -// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks > %s -fexceptions -fcxx-exceptions > +// RUN: %clang_cc1 -triple i386-apple-darwin9 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks > %s -fexceptions -fcxx-exceptions -Wno-tautological-undefined-compare > +// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks > %s -fexceptions -fcxx-exceptions -Wno-tautological-undefined-compare > > void clang_analyzer_warnIfReached(); > > > Modified: cfe/trunk/test/Analysis/reference.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/reference.cpp?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/test/Analysis/reference.cpp (original) > +++ cfe/trunk/test/Analysis/reference.cpp Fri Jun 6 16:39:26 2014 > @@ -1,4 +1,4 @@ > -// RUN: %clang_cc1 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -analyzer-constraints=range -verify > -Wno-null-dereference %s > +// RUN: %clang_cc1 -analyze > -analyzer-checker=core,alpha.core,debug.ExprInspection > -analyzer-store=region -analyzer-constraints=range -verify > -Wno-null-dereference -Wno-tautological-undefined-compare %s > > void clang_analyzer_eval(bool); > > > Modified: cfe/trunk/test/Analysis/stack-addr-ps.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/stack-addr-ps.cpp?rev=210372&r1=210371&r2=210372&view=diff > > ============================================================================== > --- cfe/trunk/test/Analysis/stack-addr-ps.cpp (original) > +++ cfe/trunk/test/Analysis/stack-addr-ps.cpp Fri Jun 6 16:39:26 2014 > @@ -1,4 +1,4 @@ > -// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-store=region > -verify %s > +// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-store=region > -verify %s -Wno-undefined-bool-conversion > > typedef __INTPTR_TYPE__ intptr_t; > > > Added: cfe/trunk/test/SemaCXX/warn-tautological-undefined-compare.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-tautological-undefined-compare.cpp?rev=210372&view=auto > > ============================================================================== > --- cfe/trunk/test/SemaCXX/warn-tautological-undefined-compare.cpp (added) > +++ cfe/trunk/test/SemaCXX/warn-tautological-undefined-compare.cpp Fri Jun > 6 16:39:26 2014 > @@ -0,0 +1,34 @@ > +// RUN: %clang_cc1 -fsyntax-only -verify %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-undefined-compare > %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-tautological-compare > -Wtautological-undefined-compare %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s > + > +void test1(int &x) { > + if (x == 1) { } > + if (&x == 0) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced null > pointer in well-defined C++ code; comparison may be assumed to always > evaluate to false}} > + if (&x != 0) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced null > pointer in well-defined C++ code; comparison may be assumed to always > evaluate to true}} > +} > + > +class test2 { > + test2() : x(y) {} > + > + void foo() { > + if (this == 0) { } > + // expected-warning@-1{{'this' pointer cannot be null in > well-defined C++ code; comparison may be assumed to always evaluate to > false}} > + if (this != 0) { } > + // expected-warning@-1{{'this' pointer cannot be null in > well-defined C++ code; comparison may be assumed to always evaluate to > true}} > + } > + > + void bar() { > + if (x == 1) { } > + if (&x == 0) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced > null pointer in well-defined C++ code; comparison may be assumed to always > evaluate to false}} > + if (&x != 0) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced > null pointer in well-defined C++ code; comparison may be assumed to always > evaluate to true}} > + } > + > + int &x; > + int y; > +}; > > Added: cfe/trunk/test/SemaCXX/warn-undefined-bool-conversion.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/warn-undefined-bool-conversion.cpp?rev=210372&view=auto > > ============================================================================== > --- cfe/trunk/test/SemaCXX/warn-undefined-bool-conversion.cpp (added) > +++ cfe/trunk/test/SemaCXX/warn-undefined-bool-conversion.cpp Fri Jun 6 > 16:39:26 2014 > @@ -0,0 +1,37 @@ > +// RUN: %clang_cc1 -fsyntax-only -verify %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wundefined-bool-conversion %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-bool-conversion > -Wundefined-bool-conversion %s > +// RUN: %clang_cc1 -fsyntax-only -verify -Wbool-conversion %s > + > +void test1(int &x) { > + if (x == 1) { } > + if (&x) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced null > pointer in well-defined C++ code; pointer may be assumed always converted > to true}} > + > + if (!&x) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced null > pointer in well-defined C++ code; pointer may be assumed always converted > to true}} > +} > + > +class test2 { > + test2() : x(y) {} > + > + void foo() { > + if (this) { } > + // expected-warning@-1{{'this' pointer cannot be null in > well-defined C++ code; pointer may be assumed always converted to true}} > + > + if (!this) { } > + // expected-warning@-1{{'this' pointer cannot be null in > well-defined C++ code; pointer may be assumed always converted to true}} > + } > + > + void bar() { > + if (x == 1) { } > + if (&x) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced > null pointer in well-defined C++ code; pointer may be assumed always > converted to true}} > + > + if (!&x) { } > + // expected-warning@-1{{reference cannot be bound to dereferenced > null pointer in well-defined C++ code; pointer may be assumed always > converted to true}} > + } > + > + int &x; > + int y; > +}; > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
