Address RFC, review feedback

Author: apple-fcloutier

clang/include/clang/Basic/AttrDocs.td

@@ -3760,20 +3760,23 @@ other. For instance:
   static int wind_speed;
 
   void say_hi(const char *fmt) {
-    printf(fmt, first_name, todays_temperature); // warning: format string is not a string literal
-    printf(fmt, first_name, wind_speed); // warning: format string is not a string literal
+    printf(fmt, first_name, todays_temperature);
+        // ^ warning: format string is not a string literal
+    printf(fmt, first_name, wind_speed);
+        // ^ warning: format string is not a string literal
   }
 
   int main() {
     say_hi("hello %s, it is %g degrees outside");
-    say_hi("hello %s, it is %d degrees outside!"); // no diagnostic
+    say_hi("hello %s, it is %d degrees outside!");
+                          // ^ no diagnostic, but %d cannot format doubles
   }
 
 In this example, ``fmt`` is expected to format a ``const char *`` and a
 ``double``, but these values are not passed to ``say_hi``. Without the
 ``format`` attribute (which cannot apply in this case), the -Wformat-nonliteral
-diagnostic triggers in the body of ``say_hi``, and incorrect ``say_hi`` call
-sites do not trigger a diagnostic.
+diagnostic unnecessarily triggers in the body of ``say_hi``, and incorrect
+``say_hi`` call sites do not trigger a diagnostic.
 
 To complement the ``format`` attribute, Clang also defines the
 ``format_matches`` attribute. Its syntax is similar to the ``format``
@@ -3803,10 +3806,31 @@ The third argument to ``format_matches`` is expected to evaluate to a **C string
 literal** even when the format string would normally be a different type for the
 given flavor, like a ``CFStringRef`` or a ``NSString *``.
 
+The only requirement on the format string literal is that it has specifiers
+that are compatible with the arguments that will be used. It can contain
+arbitrary non-format characters. For instance, for the purposes of compile-time
+validation, ``"%s scored %g%% on her test"`` and ``"%s%g"`` are interchangeable
+as the format string argument. As a means of self-documentation, users may
+prefer the former when it provides a useful example of an expected format
+string.
+
 In the implementation of a function with the ``format_matches`` attribute,
 format verification works as if the format string was identical to the one
 specified in the attribute.
 
+.. code-block:: c
+
+  __attribute__((__format_matches__(printf, 1, "%s %g")))
+  void say_hi(const char *fmt) {
+    printf(fmt, "person", 546);
+                       // ^ warning: format specifies type 'double' but the
+                       //   argument has type 'int'
+    // note: format string is defined here:
+    // __attribute__((__format_matches__(printf, 1, "%s %g")))
+    //                                                  ^~
+  }
+
+
 At the call sites of functions with the ``format_matches`` attribute, format
 verification instead compares the two format strings to evaluate their
 equivalence. Each format flavor defines equivalence between format specifiers.
@@ -3817,15 +3841,14 @@ a negative example, ``%ld`` is incompatible with ``%d``.
 
 Do note the following un-obvious cases:
 
-* Passing ``NULL`` as the format string is accepted without diagnostics.
+* Passing ``NULL`` as the format string does not trigger format diagnostics.
 * When the format string is not NULL, it cannot _miss_ specifiers, even in
   trailing positions. For instance, ``%d`` is not accepted when the required
   format is ``%d %d %d``.
-* Specifiers for integers as small or smaller than ``int`` (such as ``%hhd``)
-  are all mutually compatible because standard argument promotion ensures that
-  integers are at least the size of ``int`` when passed as variadic arguments.
-  With ``-Wformat-signedness``, mixing specifier for types with a different
-  signedness still results in a diagnostic.
+* While checks for the ``format`` attribute tolerate sone size mismatches
+  that standard argument promotion renders immaterial (such as formatting an
+  ``int`` with ``%hhd``, which specifies a ``char``-sized integer), checks for
+  ``format_matches`` require specified argument sizes to match exactly.
 * Format strings expecting a variable modifier (such as ``%*s``) are
   incompatible with format strings that would itemize the variable modifiers
   (such as ``%i %s``), even if the two specify ABI-compatible argument lists.
@@ -3837,6 +3860,12 @@ Do note the following un-obvious cases:
   This is not overridable with ``-Wformat-pedantic`` or its inverse, which
   control similar behavior in ``-Wformat``.
 
+At this time, clang implements ``format_matches`` only for format types in the
+``printf`` family. This includes variants such as Apple's NSString format and
+the FreeBSD ``kprintf``, but excludes ``scanf``. Using a known but unsupported
+format silently fails in order to be compatible with other implementations that
+would support these formats.
+
   }];
 }
 

clang/lib/AST/AttrImpl.cpp

@@ -271,7 +271,6 @@ unsigned AlignedAttr::getAlignment(ASTContext &Ctx) const {
 }
 
 StringLiteral *FormatMatchesAttr::getFormatString() const {
-  // This cannot go in headers because StringLiteral and Expr may be incomplete.
   return cast<StringLiteral>(getExpectedFormat());
 }
 

clang/lib/Sema/SemaChecking.cpp

@@ -7234,9 +7234,11 @@ void EquatableFormatArgument::VerifyCompatible(
 
   switch (ArgType.matchesArgType(S.Context, Other.ArgType)) {
   case MK::Match:
-  case MK::MatchPromotion:
     break;
 
+  case MK::MatchPromotion:
+    // Per consensus reached at https://discourse.llvm.org/t/-/83076/12,
+    // MatchPromotion is treated as a failure by format_matches.
   case MK::NoMatch:
   case MK::NoMatchTypeConfusion:
   case MK::NoMatchPromotionTypeConfusion:
@@ -8225,36 +8227,6 @@ class CheckScanfHandler : public CheckFormatHandler {
   void HandleIncompleteScanList(const char *start, const char *end) override;
 };
 
-class DecomposeScanfHandler : public CheckScanfHandler {
-  llvm::SmallVectorImpl<EquatableFormatArgument> &Specs;
-  bool HadError;
-
-  DecomposeScanfHandler(Sema &s, const FormatStringLiteral *fexpr,
-                        const Expr *origFormatExpr, Sema::FormatStringType type,
-                        unsigned firstDataArg, unsigned numDataArgs,
-                        const char *beg, Sema::FormatArgumentPassingKind APK,
-                        ArrayRef<const Expr *> Args, unsigned formatIdx,
-                        bool inFunctionCall, Sema::VariadicCallType CallType,
-                        llvm::SmallBitVector &CheckedVarArgs,
-                        UncoveredArgHandler &UncoveredArg,
-                        llvm::SmallVectorImpl<EquatableFormatArgument> &Specs)
-      : CheckScanfHandler(s, fexpr, origFormatExpr, type, firstDataArg,
-                          numDataArgs, beg, APK, Args, formatIdx,
-                          inFunctionCall, CallType, CheckedVarArgs,
-                          UncoveredArg),
-        Specs(Specs), HadError(false) {}
-
-public:
-  static bool
-  GetSpecifiers(Sema &S, const FormatStringLiteral *FSL,
-                Sema::FormatStringType type, bool InFunctionCall,
-                llvm::SmallVectorImpl<EquatableFormatArgument> &Args);
-
-  bool HandleScanfSpecifier(const analyze_scanf::ScanfSpecifier &FS,
-                            const char *startSpecifier,
-                            unsigned specifierLen) override;
-};
-
 } // namespace
 
 void CheckScanfHandler::HandleIncompleteScanList(const char *start,
@@ -8401,53 +8373,6 @@ bool CheckScanfHandler::HandleScanfSpecifier(
   return true;
 }
 
-bool DecomposeScanfHandler::GetSpecifiers(
-    Sema &S, const FormatStringLiteral *FSL, Sema::FormatStringType Type,
-    bool InFunctionCall, llvm::SmallVectorImpl<EquatableFormatArgument> &Args) {
-  StringRef Data = FSL->getString();
-  const char *Str = Data.data();
-  llvm::SmallBitVector BV;
-  UncoveredArgHandler UA;
-  DecomposeScanfHandler H(S, FSL, FSL->getFormatString(), Type, 0, 0, Str,
-                          Sema::FAPK_Elsewhere, {FSL->getFormatString()}, 0,
-                          InFunctionCall, Sema::VariadicDoesNotApply, BV, UA,
-                          Args);
-
-  if (!analyze_format_string::ParseScanfString(H, Str, Str + Data.size(),
-                                               S.getLangOpts(),
-                                               S.Context.getTargetInfo()))
-    H.DoneProcessing();
-  if (H.HadError)
-    return false;
-
-  std::sort(
-      Args.begin(), Args.end(),
-      [](const EquatableFormatArgument &A, const EquatableFormatArgument &B) {
-        return A.getPosition() < B.getPosition();
-      });
-  return true;
-}
-
-bool DecomposeScanfHandler::HandleScanfSpecifier(
-    const analyze_scanf::ScanfSpecifier &FS, const char *startSpecifier,
-    unsigned specifierLen) {
-  if (!CheckScanfHandler::HandleScanfSpecifier(FS, startSpecifier,
-                                               specifierLen)) {
-    HadError = true;
-    return false;
-  }
-
-  const auto &CS = FS.getConversionSpecifier();
-  Specs.emplace_back(
-      getSpecifierRange(startSpecifier, specifierLen),
-      getLocationOfByte(CS.getStart()), FS.getLengthModifier().getKind(),
-      CS.getCharacters(), FS.getArgType(S.Context),
-      EquatableFormatArgument::FAR_Data, EquatableFormatArgument::SS_None,
-      FS.usesPositionalArg() ? FS.getPositionalArgIndex() - 1 : Specs.size(),
-      0);
-  return true;
-}
-
 static void CompareFormatSpecifiers(Sema &S, const StringLiteral *Ref,
                                     ArrayRef<EquatableFormatArgument> RefArgs,
                                     const StringLiteral *Fmt,
@@ -8556,26 +8481,13 @@ static void CheckFormatString(
       }
     }
   } else if (Type == Sema::FST_Scanf) {
-    if (ReferenceFormatString == nullptr) {
-      CheckScanfHandler H(S, FExpr, OrigFormatExpr, Type, firstDataArg,
-                          numDataArgs, Str, APK, Args, format_idx,
-                          inFunctionCall, CallType, CheckedVarArgs,
-                          UncoveredArg);
+    CheckScanfHandler H(S, FExpr, OrigFormatExpr, Type, firstDataArg,
+                        numDataArgs, Str, APK, Args, format_idx, inFunctionCall,
+                        CallType, CheckedVarArgs, UncoveredArg);
 
-      if (!analyze_format_string::ParseScanfString(
-              H, Str, Str + StrLen, S.getLangOpts(), S.Context.getTargetInfo()))
-        H.DoneProcessing();
-    } else {
-      llvm::SmallVector<EquatableFormatArgument, 9> RefArgs, FmtArgs;
-      FormatStringLiteral RefLit = ReferenceFormatString;
-      if (DecomposeScanfHandler::GetSpecifiers(S, &RefLit, Type, inFunctionCall,
-                                               RefArgs) &&
-          DecomposeScanfHandler::GetSpecifiers(S, FExpr, Type, inFunctionCall,
-                                               FmtArgs)) {
-        CompareFormatSpecifiers(S, ReferenceFormatString, RefArgs,
-                                FExpr->getFormatString(), FmtArgs);
-      }
-    }
+    if (!analyze_format_string::ParseScanfString(
+            H, Str, Str + StrLen, S.getLangOpts(), S.Context.getTargetInfo()))
+      H.DoneProcessing();
   } // TODO: handle other formats
 }
 

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

@@ -9,9 +9,6 @@ int printf(const char *fmt, ...);
 __attribute__((format(printf, 1, 0)))
 int vprintf(const char *fmt, va_list);
 
-__attribute__((format(scanf, 1, 2)))
-int scanf(const char *fmt, ...);
-
 // MARK: -
 // Calling printf with a format from format_matches(printf) diagnoses with
 // that format string
@@ -41,30 +38,6 @@ void vformat(const char *fmt, ...) {
     va_end(ap);
 }
 
-// MARK: -
-// Calling scanf
-__attribute__((format_matches(scanf, 1, "%hhi %g"))) // expected-note 3{{comparing with this specifier}}
-void scan(const char *fmt) {
-    char i;
-    float g;
-    scanf(fmt, &i, &g);
-}
-
-__attribute__((format_matches(scanf, 1, "%hhi %g"))) // expected-note{{format string is defined here}}
-void scan2(const char *fmt) {
-    char i;
-    double g;
-    scanf(fmt, &i, &g); // expected-warning{{format specifies type 'float *' but the argument has type 'double *'}}
-}
-
-void call_scan(void) {
-    scan("%hhd %e");
-    scan("%hd %Le"); // \
-        expected-warning{{format specifier 'hd' is incompatible with 'hhi'}} \
-        expected-warning{{format specifier 'Le' is incompatible with 'g'}}
-    scan("%s %p"); // expected-warning{{format specifier 'p' is incompatible with 'g'}}
-}
-
 // MARK: -
 // Calling a function with format_matches diagnoses for incompatible formats.
 
@@ -75,7 +48,8 @@ void cvt_at(const char *c) __attribute__((format_matches(NSString, 1, "%@"))); /
     expected-note{{comparing with this format string}}
 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{{comparing with this specifier}}
-void cvt_i(const char *c) __attribute__((format_matches(printf, 1, "%i"))); // 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_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}}
@@ -86,8 +60,14 @@ void test_compatibility(void) {
     cvt_c("%u"); // expected-warning{{signedness of format specifier 'u' is incompatible with 'c'}}
     cvt_u("%c"); // expected-warning{{signedness of format specifier 'c' is incompatible with 'u'}}
 
+    cvt_i("%hi"); // expected-warning{{format specifier 'hi' is incompatible with 'i'}}
+    cvt_i("%hhi"); // expected-warning{{format specifier 'hhi' is incompatible with 'i'}}
     cvt_i("%lli"); // expected-warning{{format specifier 'lli' is incompatible with 'i'}}
     cvt_i("%p"); // expected-warning{{format specifier 'p' is incompatible with 'i'}}
+    cvt_hhi("%hhi");
+    cvt_hhi("%hi"); // expected-warning{{format specifier 'hi' is incompatible with 'hhi'}} 
+    cvt_hhi("%i"); // expected-warning{{format specifier 'i' is incompatible with 'hhi'}} 
+    cvt_hhi("%li"); // expected-warning{{format specifier 'li' is incompatible with 'hhi'}} 
     cvt_n("%s"); // expected-warning{{format specifier 's' is incompatible with 'n'}}
     cvt_s("%hhn"); // expected-warning{{format specifier 'hhn' is incompatible with 's'}}