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

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.

Reviewers: aaron.ballman

Reviewed By: aaron.ballman

Subscribers: cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70624

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -8428,7 +8428,7 @@ def warn_tautological_overlap_comparison : Warning<
 
 def warn_stringcompare : Warning<
   "result of comparison against %select{a string literal|@encode}0 is "
-  "unspecified (use strncmp instead)">,
+  "unspecified (use an explicit string comparison function instead)">,
   InGroup<StringCompare>;
 
 def warn_identity_field_assign : Warning<

clang/lib/Sema/SemaExpr.cpp

@@ -10338,7 +10338,6 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
   QualType RHSType = RHS->getType();
   if (LHSType->hasFloatingRepresentation() ||
       (LHSType->isBlockPointerType() && !BinaryOperator::isEqualityOp(Opc)) ||
-      LHS->getBeginLoc().isMacroID() || RHS->getBeginLoc().isMacroID() ||
       S.inTemplateInstantiation())
     return;
 
@@ -10366,13 +10365,18 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
     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:
@@ -10406,6 +10410,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
                                 << 1 /*array comparison*/
                                 << Result);
     }
+  }
 
   if (isa<CastExpr>(LHSStripped))
     LHSStripped = LHSStripped->IgnoreParenCasts();
@@ -10413,7 +10418,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
     RHSStripped = RHSStripped->IgnoreParenCasts();
 
   // Warn about comparisons against a string constant (unless the other
-  // operand is null); the user probably wants strcmp.
+  // operand is null); the user probably wants string comparison function.
   Expr *LiteralString = nullptr;
   Expr *LiteralStringStripped = nullptr;
   if ((isa<StringLiteral>(LHSStripped) || isa<ObjCEncodeExpr>(LHSStripped)) &&

clang/test/Sema/exprs.c

@@ -119,7 +119,7 @@ void test11(struct mystruct P, float F) {
 
 // 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 (use an explicit string comparison function instead)}}
 }
 
 int test12b(const char *X) {

clang/test/Sema/warn-stringcompare.c

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