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

9

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

10482

!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"

10482

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<
+  "comparison between NULL and null character">,
+  InGroup<DiagGroup<"pointer-compare">>, DefaultIgnore;
 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 checkNullCharToNullptr(ExprResult &E); // helper for CheckCompareOperand
   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();
 }
 
+// Purely helper function for CheckCompareOperand.
+void Sema::checkNullCharToNullptr(ExprResult &E) {
+  if (!E.get()->getType()->isAnyPointerType() &&
+      E.get()->isNullPointerConstant(Context, Expr::NPC_ValueDependentIsNotNull) ==
+        Expr::NPCK_ZeroExpression) {
+    if (isa<CharacterLiteral>(E.get())) {
+      const CharacterLiteral *RCL = dyn_cast<CharacterLiteral>(E.get());
+      if (RCL->getValue() == '\0')
+        Diag(E.get()->getExprLoc(), diag::warn_pointer_compare);
+    }
+  }
+}
+
 // 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();
     return Ty->isPointerType() || Ty->isMemberPointerType();
   };
 
   // C++2a [expr.spaceship]p6: If at least one of the operands is of pointer
   // type, array-to-pointer, ..., conversions are performed on both operands to
   // bring them to their composite type.
   // Otherwise, all comparisons expect an rvalue, so convert to rvalue before
   // any type-related checks.
   if (!IsThreeWay || IsAnyPointerType(LHS) || IsAnyPointerType(RHS)) {
     LHS = DefaultFunctionArrayLvalueConversion(LHS.get());
     if (LHS.isInvalid())
       return QualType();
     RHS = DefaultFunctionArrayLvalueConversion(RHS.get());
     if (RHS.isInvalid())
       return QualType();
   } else {
     LHS = DefaultLvalueConversion(LHS.get());
     if (LHS.isInvalid())
       return QualType();
     RHS = DefaultLvalueConversion(RHS.get());
     if (RHS.isInvalid())
       return QualType();
   }
 
   checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/true);
+  checkNullCharToNullptr(LHS);
+  checkNullCharToNullptr(RHS);
 
   // 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 -Wpointer-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -DSILENCE -Wall %s
+
+#ifdef SILENCE
+  // expected-no-diagnostics
+#endif
+
+int test1(int *a) {
+  return a == '\0';
+#ifndef SILENCE
+  // expected-warning@-2 {{comparison between NULL and null character}}
+#endif
+}
+
+int test2(int *a) {
+  return '\0' == a;
+#ifndef SILENCE
+  // expected-warning@-2 {{comparison between NULL and null character}}
+#endif
+}