[Diagnostics] Warn for comparison with string literals expanded from macro (PR44064)

[xbolva00]

Summary

As noted in PR, we have a poor test coverage for this warning. I think macro support was just overlooked. GCC warns in these cases.
Clang missed a real bug in the code I am working with, GCC caught it.

[xbolva00]

clang/test/Sema/warn-stringcompare.c

4–6

Current behaviour: https://godbolt.org/z/7nYH6N

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8431

I think this is a regression in the diagnostic because we dropped the recommendation for how to fix the issue. Why do we need to drop that? I'd be fine if it was made less specific, like: (use an explicit string comparison function instead) if the concern is over strncmp specifically.

clang/lib/Sema/SemaExpr.cpp

10368

Why do we care about the macros here? It seems just as reasonable to warn on:

#define MACRO1 "one"
#define MACRO2 "one

if (MACRO1 == MACRO2) // Why not warn here too?

[xbolva00]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8431

Yes, my concern was about strncmp. Your suggestion is fine.

clang/lib/Sema/SemaExpr.cpp

10368

This “macro loc” check is for array comparison diagnostic. I preserved original logic of this function and enabled -Wstring-compare for macro locations.

We warn here (I will add a test).

[aaron.ballman]

clang/lib/Sema/SemaExpr.cpp

10368

Ah, thank you!

[xbolva00]

Added new tests.
Improved recommendation how to fix warning.

[aaron.ballman]

LGTM!

[xbolva00]

Thank you for the review

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_comparison_bitwise_or : Warning<
   "bitwise or with non-zero value always evaluates to true">,
   InGroup<TautologicalBitwiseCompare>, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<
   "overlapping comparisons always evaluate to %select{false|true}0">,
   InGroup<TautologicalOverlapCompare>, DefaultIgnore;
 
 def warn_stringcompare : Warning<
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
-  InGroup<StringCompare>;
+  "unspecified">, InGroup<StringCompare>;
 
 def warn_identity_field_assign : Warning<
   "assigning %select{field|instance variable}0 to itself">,
   InGroup<SelfAssignmentField>;
 
 // Type safety attributes
 def err_type_tag_for_datatype_not_ice : Error<
   "'type_tag_for_datatype' attribute requires the initializer to be "

clang/lib/Sema/SemaExpr.cpp

static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                                            BinaryOperatorKind Opc) {
   Expr *LHSStripped = LHS->IgnoreParenImpCasts();
   Expr *RHSStripped = RHS->IgnoreParenImpCasts();
 
   QualType LHSType = LHS->getType();
   QualType RHSType = RHS->getType();
   if (LHSType->hasFloatingRepresentation() ||
       (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) ||
-      LHS->getBeginLoc().isMacroID() || RHS->getBeginLoc().isMacroID() ||
       S.inTemplateInstantiation())
     return;
 
   // Comparisons between two array types are ill-formed for operator<=>, so
   // we shouldn't emit any additional warnings about it.
   if (Opc == BO_Cmp && LHSType->isArrayType() && RHSType->isArrayType())
     return;
 
static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
   // Used for indexing into %select in warn_comparison_always
   enum {
     AlwaysConstant,
     AlwaysTrue,
     AlwaysFalse,
     AlwaysEqual, // std::strong_ordering::equal from operator<=>
   };
 
+  if (!LHS->getBeginLoc().isMacroID() && !RHS->getBeginLoc().isMacroID()) {
     if (Expr::isSameComparisonOperand(LHS, RHS)) {
       unsigned Result;
       switch (Opc) {
-    case BO_EQ: case BO_LE: case BO_GE:
+      case BO_EQ:
+      case BO_LE:
+      case BO_GE:
         Result = AlwaysTrue;
         break;
-    case BO_NE: case BO_LT: case BO_GT:
+      case BO_NE:
+      case BO_LT:
+      case BO_GT:
         Result = AlwaysFalse;
         break;
       case BO_Cmp:
         Result = AlwaysEqual;
         break;
       default:
         Result = AlwaysConstant;
         break;
       }
       S.DiagRuntimeBehavior(Loc, nullptr,
                             S.PDiag(diag::warn_comparison_always)
                                 << 0 /*self-comparison*/
                                 << Result);
     } else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
       // What is it always going to evaluate to?
       unsigned Result;
-    switch(Opc) {
+      switch (Opc) {
       case BO_EQ: // e.g. array1 == array2
         Result = AlwaysFalse;
         break;
       case BO_NE: // e.g. array1 != array2
         Result = AlwaysTrue;
         break;
       default: // e.g. array1 <= array2
         // The best we can say is 'a constant'
         Result = AlwaysConstant;
         break;
       }
       S.DiagRuntimeBehavior(Loc, nullptr,
                             S.PDiag(diag::warn_comparison_always)
                                 << 1 /*array comparison*/
                                 << Result);
     }
+  }
 
   if (isa<CastExpr>(LHSStripped))
     LHSStripped = LHSStripped->IgnoreParenCasts();
   if (isa<CastExpr>(RHSStripped))
     RHSStripped = RHSStripped->IgnoreParenCasts();
 
   // Warn about comparisons against a string constant (unless the other
   // operand is null); the user probably wants strcmp.

clang/test/Sema/exprs.c

 
 struct mystruct {int A; };
 void test11(struct mystruct P, float F) {
   MYMAX(P, F);  // expected-error {{invalid operands to binary expression ('typeof (P)' (aka 'struct mystruct') and 'typeof (F)' (aka 'float'))}}
 }
 
 // PR3753
 int test12(const char *X) {
-  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+  return X == "foo";  // expected-warning {{comparison against a string literal is unspecified}}
 }
 
 int test12b(const char *X) {
   return sizeof(X == "foo"); // no-warning
 }
 
 // rdar://6719156
 void test13(

clang/test/Sema/warn-stringcompare.c

+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+
+#define DELIM "/"
+#define NULL (void *)0
+
+void test(const char *d) {
+  if ("/" != d) // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if (d == "/") // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if ("/" != NULL)
+    return;
+  if (NULL == "/")
+    return;
+  if ("/" != DELIM) // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+  if (DELIM == "/") // expected-warning {{result of comparison against a string literal is unspecified}}
+    return;
+}