Add -Wpointer-compare

[ziyig]

Summary

Add -Wpointer-compare to check instances of comparing a Null pointer to '\0'.

[xbolva00]

+1, useful check.

what about:
char *a...
(char) 0 == a

?

Is this off by default in GCC? I would like to see it -Wall/-Wextra..

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

3291

I think this would read a bit better as comparing a pointer to a null character constant; did you mean to compare to %select{nullptr|NULL|0}?

I'm a bit unsure about NULL vs 0 in the diagnostic, but I kind of feel like we should prefer nullptr if it's enabled, NULL if it's available, and 0 only as a last-ditch fallback.

3292

Why is this disabled by default?

clang/include/clang/Sema/Sema.h

9994

Should be CheckNullCharToNullptr() to fit with the surrounding naming conventions.

clang/lib/Sema/SemaExpr.cpp

10436

I'd drop this comment as it doesn't add much value.

10441

Rather than do isa<> followed by a dyn_cast<>, this should be:

if (const auto *RCL = dyn_cast<CharacterLiteral>(E.get())) {
}

10443

I think it would be more clear to compare against 0 as an integer literal here.

clang/test/Sema/warn-nullchar-nullptr.c

10

I'd appreciate tests for the other character literals as well. L'\0', u8'\0', etc.

[ziyig]

In D65595#1610762, @xbolva00 wrote:

+1, useful check.

what about:
char *a...
(char) 0 == a

?

Thanks for the suggestion, I added the test.

Is this off by default in GCC? I would like to see it -Wall/-Wextra..

This is not off by default in GCC, changed.

clang/include/clang/Basic/DiagnosticSemaKinds.td

3292

I changed this to not off by default.

[aaron.ballman]

clang/include/clang/Sema/Sema.h

9994

I'd drop the comment here, it doesn't add much value (and it's over the 80 col limit).

clang/lib/Sema/SemaExpr.cpp

10447

This logic is incorrect (and it won't pick from three different options provided by the diagnostic).

10448–10449

Formatting is off here -- you should run the patch through clang-format, if you don't already do that.

10451

This should probably be looking at the canonical type so that we strip off all the sugar. You should add a test like:

typedef char my_char;

int func(int *a) {
  return a == (my_char)0; 
}

10452

I see that GCC only warns on char types, but it seems equally plausible (through macros or typedefs) to get a wchar_t or other kind of character type. Should we warn on those as well?

clang/test/Sema/warn-nullchar-nullptr.c

5

We definitely should not be suggesting nullptr in C code. :-)

[xbolva00]

Maybe emit fixit with NULL/nullptr too?

[ziyig]

clang/lib/Sema/SemaExpr.cpp

10452

I don't have any opinion about this. Which one do you think is better?

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

10440

StringRef?

10461

You want to replace it with NullValue, not NullNotes.

[xbolva00]

clang/test/Sema/warn-nullchar-nullptr.c

6

Please add test for:

a != ‘\0’

[ziyig]

clang/lib/Sema/SemaExpr.cpp

10452

It works for typedefs and MACRO now. Is this what you expected?

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

10460

You can ditch SelectValue and use %0 in diag msg. Then ‘<< NullValue’.

[xbolva00]

Can you add tests for C++? E.g C++03 and C++11?

Other than that, I have no comments - looks fine.

[ziyig]

In D65595#1612812, @xbolva00 wrote:

Can you add tests for C++? E.g C++03 and C++11?

C++ doesn't support comparison between pointer and character, so I guess it's not necessary?
Here is an example of comparing pointer and character in C++:
int foo(int *a) {

return a == '\0';

}

#1 with x86-64 clang (trunk)
<source>:2:14: error: comparison between pointer and integer ('int *' and 'char')

return a == '\0';
       ~ ^  ~~~~

1 error generated.
Compiler returned: 1

[xbolva00]

So

if (C mode only) {
CheckNullCharToNullptr(LHS, RHS);
CheckNullCharToNullptr(RHS, LHS);
}

?

Drop if (cpp11) condition in the function?

Rename it please. ‘CheckPtrComparisonWithNullChar’ ?

[xbolva00]

clang/lib/Sema/SemaExpr.cpp

10497

!getLangOpts().CPlusPlus ?

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

10439–10443

I would rather see this done using a %select in the diagnostic, since both of the strings are statically known at compile time.

10452

Not quite -- I wasn't very clear. I was thinking more about a scenario like this:

#define CHECK_VAL(x, y) (x != y)
#ifdef NARROW
#define TYPE char
#else
#define TYPE wchar_t
#endif

void func(int *ip) {
  if (CHECK_VAL(ip, (TYPE)0))
    ;
}

When NARROW is defined, your check will trigger, but when it's not defined, there is no diagnostic.

I can't think of a situation where we'd want to warn on a cast involving char but not wchar_t (for instance), so I think you should check for isAnyCharacterType() rather than just CharTy below.

10453

Formatting is still off here (the else binds to the } above).

10459

When switching to using %select above, this can become NullValue ? "NULL" : "(void *)0"

10497

Agreed -- that's the canonical way to check for any C mode.

[xbolva00]

@aaron.ballman, Should we also consider address spaces? Or exotic NULL as non-zero bit pattern?

[aaron.ballman]

In D65595#1614775, @xbolva00 wrote:

@aaron.ballman, Should we also consider address spaces? Or exotic NULL as non-zero bit pattern?

I don't think address spaces would have any impact here, and unless LLVM supports a platform with a non-zero NULL bit pattern, I don't think it would be all that beneficial to consider it (we could always do that in a follow-up patch though).

[xbolva00]

Right, thanks for info.

[ziyig]

clang/lib/Sema/SemaExpr.cpp

10452

I changed the check to isAnyCharacterType(), but I don't think there's any way to test it in C cause wchar_t will be treated as an interger in C, so I didn't add tests for this.

[xbolva00]

a > (char) 0 deserves other diagnostic message.. gcc emits: ordered comparison of pointer with integer zero.

This warning should handle ==, != only, I think.

Pleae add test for a > ‘\0’ to be sure we dont suggest a > NULL.
Then a follow up patch could diagnose ordered comparison with integer zero.

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

10439–10443

int NullValue = PP.isMacroDefined("NULL") ? 0 : 1;

10452

Ugh, so that explains why this part of the check only makes sense for char types. I forgot that wchar_t has an underlying integer type in C. I guess you can go back to just checking CharTy because the same holds true for char16_t and char32_t in C. Sorry about the churn!

[aaron.ballman]

LGTM, thank you for the patch!

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_impcast_different_enum_types : Warning<
   "implicit conversion from enumeration type %0 to different enumeration type "
   "%1">, InGroup<EnumConversion>;
 def warn_impcast_bool_to_null_pointer : Warning<
     "initialization of pointer of type %0 to null from a constant boolean "
     "expression">, InGroup<BoolConversion>;
 def warn_non_literal_null_pointer : Warning<
     "expression which evaluates to zero treated as a null pointer constant of "
     "type %0">, InGroup<NonLiteralNullConversion>;
+def warn_pointer_compare : Warning<
+    "comparing a pointer to a null character constant; did you mean "
+    "to compare to %0?">,
+    InGroup<DiagGroup<"pointer-compare">>;
 def warn_impcast_null_pointer_to_integer : Warning<
     "implicit conversion of %select{NULL|nullptr}0 constant to %1">,
     InGroup<NullConversion>;
 def warn_impcast_floating_point_to_bool : Warning<
     "implicit conversion turns floating-point number into bool: %0 to %1">,
     InGroup<ImplicitConversionFloatingPointToBool>;
 def ext_ms_impcast_fn_obj : ExtWarn<
   "implicit conversion between pointer-to-function and pointer-to-object is a "

clang/include/clang/Sema/Sema.h

QualType CheckAdditionOperands( // C99 6.5.6
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc, QualType* CompLHSTy = nullptr);
   QualType CheckSubtractionOperands( // C99 6.5.6
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     QualType* CompLHSTy = nullptr);
   QualType CheckShiftOperands( // C99 6.5.7
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
     BinaryOperatorKind Opc, bool IsCompAssign = false);
+  void CheckPtrComparisonWithNullChar(ExprResult &E, ExprResult &NullE);
   QualType CheckCompareOperands( // C99 6.5.8/9
       ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
       BinaryOperatorKind Opc);
   QualType CheckBitwiseOperands( // C99 6.5.[10...12]
       ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,
       BinaryOperatorKind Opc);
   QualType CheckLogicalOperands( // C99 6.5.[13,14]
     ExprResult &LHS, ExprResult &RHS, SourceLocation Loc,

clang/lib/Sema/SemaExpr.cpp

static QualType checkArithmeticOrEnumeralCompare(Sema &S, ExprResult &LHS,
   // Check for comparisons of floating point operands using != and ==.
   if (Type->hasFloatingRepresentation() && BinaryOperator::isEqualityOp(Opc))
     S.CheckFloatComparison(Loc, LHS.get(), RHS.get());
 
   // The result of comparisons is 'bool' in C++, 'int' in C.
   return S.Context.getLogicalOperationType();
 }
 
+void Sema::CheckPtrComparisonWithNullChar(ExprResult &E, ExprResult &NullE) {
+  if (!NullE.get()->getType()->isAnyPointerType())
+    return;
+  StringRef NullValue;
+  if (PP.isMacroDefined("NULL"))
+    NullValue = "NULL";
+  else
+    NullValue = "(void *) 0";
+  if (!E.get()->getType()->isAnyPointerType() &&
+      E.get()->isNullPointerConstant(Context, Expr::NPC_ValueDependentIsNotNull) ==
+        Expr::NPCK_ZeroExpression) {
+    if (const auto *CL = dyn_cast<CharacterLiteral>(E.get())) {
+      if (CL->getValue() == 0)
+        Diag(E.get()->getExprLoc(), diag::warn_pointer_compare)
+            << NullValue
+            << FixItHint::CreateReplacement(E.get()->getExprLoc(), NullValue);
+    }
+      else if (const auto *CE = dyn_cast<CStyleCastExpr>(E.get())) {
+        TypeSourceInfo *TI = CE->getTypeInfoAsWritten();
+        QualType T = Context.getCanonicalType(TI->getType()).getUnqualifiedType();
+        if (T == Context.CharTy)
+          Diag(E.get()->getExprLoc(), diag::warn_pointer_compare)
+              << NullValue
+              << FixItHint::CreateReplacement(E.get()->getExprLoc(), NullValue);
+      }
+  }
+}
+
 // C99 6.5.8, C++ [expr.rel]
 QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
                                     SourceLocation Loc,
                                     BinaryOperatorKind Opc) {
   bool IsRelational = BinaryOperator::isRelationalOp(Opc);
   bool IsThreeWay = Opc == BO_Cmp;
   auto IsAnyPointerType = [](ExprResult E) {
     QualType Ty = E.get()->getType();
if (!IsThreeWay || IsAnyPointerType(LHS) || IsAnyPointerType(RHS)) {
     if (LHS.isInvalid())
       return QualType();
     RHS = DefaultLvalueConversion(RHS.get());
     if (RHS.isInvalid())
       return QualType();
   }
 
   checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/true);
+  if (getLangOpts().C99) {
+    CheckPtrComparisonWithNullChar(LHS, RHS);
+    CheckPtrComparisonWithNullChar(RHS, LHS);
+  }
 
   // Handle vector comparisons separately.
   if (LHS.get()->getType()->isVectorType() ||
       RHS.get()->getType()->isVectorType())
     return CheckVectorCompareOperands(LHS, RHS, Loc, Opc);
 
   diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc);
   diagnoseTautologicalComparison(*this, Loc, LHS.get(), RHS.get(), Opc);

clang/test/Sema/warn-nullchar-nullptr.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Wall %s
+
+int test1(int *a) {
+  return a == '\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test2(int *a) {
+  return '\0' == a; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test3(int *a) {
+  return a == L'\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test4(int *a) {
+  return a == u'\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test5(int *a) {
+  return a == U'\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test6(int *a) {
+  return a == (char)0; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+typedef char my_char;
+int test7(int *a) {
+  return a == (my_char)0;
+  // expected-warning@-1 {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+int test8(int *a) {
+  return a != '\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to (void *) 0?}}
+}
+
+#define NULL (void *)0
+int test9(int *a) {
+  return a == '\0'; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to NULL?}}
+}
+
+#define MYCHAR char
+int test10(int *a) {
+  return a == (MYCHAR)0; // expected-warning {{comparing a pointer to a null character constant; did you mean to compare to NULL?}}
+}