Add warnings for undefined behaviors with pointers

[rtrieu]

Summary

Extend tautological compare and bool conversion warnings to undefined pointer operations. These are cases where all defined behavior will give the expression one value, while the other value can only result from undefined behavior, which the optimizer can remove. These cases are addresses of references and the this pointer. This warning was requested in PR19899 in response to commit r209723.

[rnk]

include/clang/Basic/DiagnosticSemaKinds.td

2394

Should these really all be DefaultIgnore if we're optimizing these checks away?

[rsmith]

Some suggestions on diagnostic wording.

include/clang/Basic/DiagnosticSemaKinds.td

2392–2393

"undefined contexts" is rather unclear here. Maybe something like:

"'this' pointer cannot be null in well-defined C++ code; comparison may be removed"

(I don't like "during optimization" either, since it suggests we guarantee to leave this alone at -O0)

2396–2398

Likewise:

"reference cannot be bound to dereferenced null pointer in well-defined C++ code; comparison may be removed"

[rtrieu]

Make warnings on by default, change wording of warnings, disable warnings in some analysis tests.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

2392–2393

Typo 'covnerted'.

Maybe "pointer may be assumed to always convert to 'true'"?

2395

Typo "dereference", "well-define".

2405

Perhaps "may be assumed to always evaluate to %select{...}".

[rtrieu]

Fix typos and update wording.

[rsmith]

LGTM with some minor diagnostic tweaks.

include/clang/Basic/DiagnosticSemaKinds.td

2393

"assumed to always convert to" would sound better to me.

Also,

assumed to always convert to 'true'

rather than

assumed to always convert to a true value

to avoid people wondering what "a true value" is (cf. "a true Scotsman").

Likewise I'd suggest putting the %select's below into single quotes.

2409

Typo "maybe be" should be "may be"

[rtrieu]

Committed in r210372

include/clang/Basic/DiagnosticGroups.td

 def GNUCompoundLiteralInitializer : DiagGroup<"gnu-compound-literal-initializer">;
 def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion">;
 def ConstantConversion :
   DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
 def LiteralConversion : DiagGroup<"literal-conversion">;
 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">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
 def NonLiteralNullConversion : DiagGroup<"non-literal-null-conversion">;
 def NullConversion : DiagGroup<"null-conversion">;
 def ImplicitConversionFloatingPointToBool :
   DiagGroup<"implicit-conversion-floating-point-to-bool">;

 def GNUStatementExpression : DiagGroup<"gnu-statement-expression">;
 def StringCompare : DiagGroup<"string-compare">;
 def StringPlusInt : DiagGroup<"string-plus-int">;
 def StringPlusChar : DiagGroup<"string-plus-char">;
 def StrncatSize : DiagGroup<"strncat-size">;
 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">;
 def GNUUnionCast : DiagGroup<"gnu-union-cast">;
 def GNUVariableSizedTypeNotAtEnd : DiagGroup<"gnu-variable-sized-type-not-at-end">;
 def Varargs : DiagGroup<"varargs">;
 
 def Unsequenced : DiagGroup<"unsequenced">;

include/clang/Basic/DiagnosticSemaKinds.td

 def warn_impcast_floating_point_to_bool : Warning<
     "implicit conversion turns floating-point number into bool: %0 to %1">,
     InGroup<ImplicitConversionFloatingPointToBool>;
 
 def warn_impcast_pointer_to_bool : Warning<
     "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 a true value">, 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 a true value">,
+  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 maybe 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">;
 def note_function_to_function_call : Note<
     "suffix with parentheses to turn this into a function call">;
 def warn_impcast_objective_c_literal_to_bool : Warning<
     "implicit boolean conversion of Objective-C object literal always "
     "evaluates to true">,

lib/Sema/SemaChecking.cpp

void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
 
   // Don't warn inside macros.
   if (E->getExprLoc().isMacroID())
       return;
   E = E->IgnoreImpCasts();
 
   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)) {
     if (UO->getOpcode() != UO_AddrOf)
       return;
     IsAddressOf = true;
     E = UO->getSubExpr();
   }
void Sema::DiagnoseAlwaysNonNullPointer(Expr *E,
   QualType T = D->getType();
   const bool IsArray = T->isArrayType();
   const bool IsFunction = T->isFunctionType();
 
   if (IsAddressOf) {
     // 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.
   if (!IsAddressOf && !IsFunction && !IsArray)
     return;
 
   // Pretty print the expression for the diagnostic.
   std::string Str;
   llvm::raw_string_ostream S(Str);

test/Analysis/inlining/path-notes.cpp

-// 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 {
 public:
   static void use(int *p) {
     *p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
     // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
   }

test/Analysis/misc-ps-region-store.cpp

-// 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();
 
 // Test basic handling of references.
 char &test1_aux();
 char *test1() {
   return &test1_aux();
 }

test/Analysis/reference.cpp

-// 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);
 
 typedef typeof(sizeof(int)) size_t;
 void malloc (size_t);
 
 void f1() {
   int const &i = 3;

test/Analysis/stack-addr-ps.cpp

-// 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;
 
 const int& g() {
   int s;
   return s; // expected-warning{{Address of stack memory associated with local variable 's' returned}} expected-warning{{reference to stack memory associated with local variable 's' returned}}
 }
 

test/SemaCXX/warn-tautological-undefined-compare.cpp

+// 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 maybe 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 maybe 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 maybe 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 maybe be assumed to always evaluate to true}}
+  }
+
+  int &x;
+  int y;
+};

test/SemaCXX/warn-undefined-bool-conversion.cpp

+// 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 a true value}}
+
+  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 a true value}}
+}
+
+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 a true value}}
+
+    if (!this) { }
+    // expected-warning@-1{{'this' pointer cannot be null in well-defined C++ code; pointer may be assumed always converted to a true value}}
+  }
+
+  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 a true value}}
+
+    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 a true value}}
+  }
+
+  int &x;
+  int y;
+};