Add test for warn_format_cmp_modifierfor_mismatch

Author: apple-fcloutier

clang/lib/Sema/SemaChecking.cpp

@@ -7407,25 +7407,7 @@ bool DecomposePrintfHandler::GetSpecifiers(
       [](const EquatableFormatArgument &A, const EquatableFormatArgument &B) {
         return A.getPosition() < B.getPosition();
       });
-
-  // If there are duplicate positional arguments, ensure that they are all
-  // mutually compatible.
-  bool HadError = false;
-  auto ArgIter = Args.begin();
-  auto ArgEnd = Args.end();
-  while (ArgIter != ArgEnd) {
-    auto ArgNext = ArgIter + 1;
-    auto PositionalEnd = std::find_if(ArgNext, ArgEnd, [=](const auto &Arg) {
-      return Arg.getPosition() != ArgIter->getPosition();
-    });
-    while (ArgNext != PositionalEnd) {
-      HadError |=
-          !ArgIter->VerifyCompatible(S, *ArgNext, FmtExpr, InFunctionCall);
-      ++ArgNext;
-    }
-    ArgIter = ArgNext;
-  }
-  return !HadError;
+  return true;
 }
 
 bool DecomposePrintfHandler::HandlePrintfSpecifier(
@@ -8514,36 +8496,58 @@ static bool CompareFormatSpecifiers(Sema &S, const StringLiteral *Ref,
                                     ArrayRef<EquatableFormatArgument> FmtArgs,
                                     const Expr *FmtExpr, bool InFunctionCall) {
   bool HadError = false;
-  unsigned CommonArgs;
-  if (FmtArgs.size() > RefArgs.size()) {
-    CommonArgs = RefArgs.size();
-    // "data argument not used by format string"
+  auto FmtIter = FmtArgs.begin(), FmtEnd = FmtArgs.end();
+  auto RefIter = RefArgs.begin(), RefEnd = RefArgs.end();
+  while (FmtIter < FmtEnd && RefIter < RefEnd) {
+    // In positional-style format strings, the same specifier can appear
+    // multiple times. In the Ref format string, we have already verified that
+    // each repetition of the same positional specifier are mutually compatible,
+    // so we only need to test each repetition in the Fmt string with the first
+    // corresponding Ref specifier.
+    auto FmtIterEnd = std::find_if(FmtIter + 1, FmtEnd, [=](const auto &Arg) {
+      return Arg.getPosition() != FmtIter->getPosition();
+    });
+    auto RefIterEnd = std::find_if(RefIter + 1, RefEnd, [=](const auto &Arg) {
+      return Arg.getPosition() != RefIter->getPosition();
+    });
+
+    // Clang does not diagnose missing format specifiers in positional-style
+    // strings (TODO: which it probably should do, as it is UB to skip over a
+    // format argument). Skip specifiers if needed.
+    if (FmtIter->getPosition() < RefIter->getPosition()) {
+      FmtIter = FmtIterEnd;
+      continue;
+    }
+    if (RefIter->getPosition() < FmtIter->getPosition()) {
+      RefIter = RefIterEnd;
+      continue;
+    }
+
+    while (FmtIter != FmtIterEnd) {
+      HadError |=
+          !FmtIter->VerifyCompatible(S, *RefIter, FmtExpr, InFunctionCall);
+      ++FmtIter;
+    }
+    RefIter = RefIterEnd;
+  }
+
+  if (FmtIter < FmtEnd) {
     CheckFormatHandler::EmitFormatDiagnostic(
         S, InFunctionCall, FmtExpr,
         S.PDiag(diag::warn_format_cmp_specifier_arity) << 1,
-        FmtExpr->getBeginLoc(), false,
-        FmtArgs[RefArgs.size()].getSourceRange());
+        FmtExpr->getBeginLoc(), false, FmtIter->getSourceRange());
     S.Diag(Ref->getBeginLoc(), diag::note_format_cmp_with) << 1;
     HadError = true;
-  } else if (RefArgs.size() > FmtArgs.size()) {
-    CommonArgs = FmtArgs.size();
-    // "fewer specifiers in format string than expected"
+  } else if (RefIter < RefEnd) {
     CheckFormatHandler::EmitFormatDiagnostic(
         S, InFunctionCall, FmtExpr,
         S.PDiag(diag::warn_format_cmp_specifier_arity) << 0,
         FmtExpr->getBeginLoc(), false, Fmt->getSourceRange());
     S.Diag(Ref->getBeginLoc(), diag::note_format_cmp_with)
-        << 1 << RefArgs[FmtArgs.size()].getSourceRange();
+        << 1 << RefIter->getSourceRange();
     HadError = true;
-  } else {
-    CommonArgs = FmtArgs.size();
   }
-
-  for (size_t I = 0; I < CommonArgs; ++I) {
-    HadError |=
-        !FmtArgs[I].VerifyCompatible(S, RefArgs[I], FmtExpr, InFunctionCall);
-  }
-  return HadError;
+  return !HadError;
 }
 
 static void CheckFormatString(

clang/test/Sema/format-string-matches.c

@@ -60,7 +60,7 @@ void cvt_at(const char *c) __attribute__((format_matches(NSString, 1, "%@")));
 void cvt_c(const char *c) __attribute__((format_matches(printf, 1, "%c"))); // expected-note{{comparing with this specifier}}
 void cvt_u(const char *c) __attribute__((format_matches(printf, 1, "%u"))); // expected-note 2{{comparing with this specifier}}
 void cvt_hhi(const char *c) __attribute__((format_matches(printf, 1, "%hhi")));  // expected-note 3{{comparing with this specifier}}
-void cvt_i(const char *c) __attribute__((format_matches(printf, 1, "%i"))); // expected-note 4{{comparing with this specifier}}
+void cvt_i(const char *c) __attribute__((format_matches(printf, 1, "%i"))); // expected-note 5{{comparing with this specifier}}
 void cvt_p(const char *c) __attribute__((format_matches(printf, 1, "%p")));
 void cvt_s(const char *c) __attribute__((format_matches(printf, 1, "%s"))); // expected-note{{comparing with this specifier}}
 void cvt_n(const char *c) __attribute__((format_matches(printf, 1, "%n"))); // expected-note{{comparing with this specifier}}
@@ -223,11 +223,24 @@ void test_merge_redecl_warn(const char *f);
 // Positional madness
 
 __attribute__((format_matches(printf, 1, "%1$s %1$d")))  // \
-    expected-warning{{format specifier 's' is incompatible with 'd'}} \
+    expected-warning{{format specifier 'd' is incompatible with 's'}} \
     expected-note{{comparing with this specifier}}
 void test_positional_incompatible(const char *f);
 
 void call_positional_incompatible(void) {
     // expect the attribute was dropped and that there is no diagnostic here
     test_positional_incompatible("%d %d %d %d %d");
-}
+}
+
+void test_many_i(void) {
+    cvt_i("%1$d %1$i");
+    cvt_i("%1$d %1$s"); // expected-warning{{format specifier 's' is incompatible with 'i'}}
+}
+
+__attribute__((format_matches(printf, 1, "%*d %*d"))) // expected-note{{comparing with this specifier}}
+void accept_modifiers(const char *f);
+
+void test_modifiers(void) {
+    accept_modifiers("%2$*1$d %4$*3$d");
+    accept_modifiers("%2$*3$d %4$*3$d"); // expected-warning{{format argument modifies specifier at position 2, but it should modify specifier at position 4}}
+}