[Sema] Split out -Wformat-type-confusion from -Wformat-pedantic

[erik.pilkington]

Summary

The warnings now in -Wformat-type-confusion don't align with how we interpret 'pedantic' in clang, and don't belong in -pedantic.

Suggested by Aaron here: D66856

Thanks for taking a look!
Erik

[erik.pilkington]

Ping!

[aaron.ballman]

In D67775#1691999, @erik.pilkington wrote:

Ping!

Sorry for the delayed review -- thank you for working on clearing this up!

clang/include/clang/AST/FormatString.h

259 ↗

(On Diff #220902)

Can you add: For instance, "%d" and _Bool.

clang/lib/Sema/SemaChecking.cpp

8165 ↗

(On Diff #220902)

How about:

if (ImplicitMatch == ArgType::NoMatchPedantic ||
    ImplicitMatch == ArgType::NoMatchTypeConfusion)
  Match = ImplicitMatch;

clang/test/Sema/format-strings-pedantic.c

1 ↗

(On Diff #220902)

Are we losing test coverage for -Wformat-pedantic, or do we have other tests covering that elsewhere? I would have expected this test file's contents to exercise pedantic cases.

clang/test/Sema/format-type-confusion.c

13 ↗

(On Diff #220902)

Just double-checking, but the reason we don't diagnose the %c here is because of -Wno-format?

[erik.pilkington]

Address review comments.

clang/test/Sema/format-strings-pedantic.c

1 ↗

(On Diff #220902)

The only warning that was in this file is now under -Wformat-type-confusion. New patch adds a test for the printf("%p", (int*)0); thing, which was otherwise untested.

clang/test/Sema/format-type-confusion.c

13 ↗

(On Diff #220902)

Yup, exactly. I just wanted to test -Wformat-type-confusion alone in this file.

[aaron.ballman]

LGTM, thank you for this!

clang/test/Sema/format-strings-pedantic.c

1 ↗

(On Diff #220902)

Thank you for adding that!

clang/test/Sema/format-type-confusion.c

13 ↗

(On Diff #220902)

Awesome, thank you for the verification!

cfe/trunk/include/clang/AST/FormatString.h

protected:
   Kind kind;
 };
 
 class ArgType {
 public:
   enum Kind { UnknownTy, InvalidTy, SpecificTy, ObjCPointerTy, CPointerTy,
               AnyCharTy, CStrTy, WCStrTy, WIntTy };
 
-  enum MatchKind { NoMatch = 0, Match = 1, NoMatchPedantic };
+  /// How well a given conversion specifier matches its argument.
+  enum MatchKind {
+    /// The conversion specifier and the argument types are incompatible. For
+    /// instance, "%d" and float.
+    NoMatch = 0,
+    /// The conversion specifier and the argument type are compatible. For
+    /// instance, "%d" and _Bool.
+    Match = 1,
+    /// The conversion specifier and the argument type are disallowed by the C
+    /// standard, but are in practice harmless. For instance, "%p" and int*.
+    NoMatchPedantic,
+    /// The conversion specifier and the argument type are compatible, but still
+    /// seems likely to be an error. For instance, "%hd" and _Bool.
+    NoMatchTypeConfusion,
+  };
 
 private:
   const Kind K;
   QualType T;
   const char *Name = nullptr;
   bool Ptr = false;
 
   /// The TypeKind identifies certain well-known types like size_t and

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

def Unused : DiagGroup<"unused",
                         DiagCategory<"Unused Entity Issue">;
 
 // Format settings.
 def FormatInvalidSpecifier : DiagGroup<"format-invalid-specifier">;
 def FormatSecurity : DiagGroup<"format-security">;
 def FormatNonStandard : DiagGroup<"format-non-iso">;
 def FormatY2K : DiagGroup<"format-y2k">;
 def FormatPedantic : DiagGroup<"format-pedantic">;
+def FormatTypeConfusion : DiagGroup<"format-type-confusion">;
 def Format : DiagGroup<"format",
                        [FormatExtraArgs, FormatZeroLength, NonNull,
                         FormatSecurity, FormatY2K, FormatInvalidSpecifier]>,
              DiagCategory<"Format String Issue">;
 def FormatNonLiteral : DiagGroup<"format-nonliteral">;
 def Format2 : DiagGroup<"format=2",
                         [FormatNonLiteral, FormatSecurity, FormatY2K]>;
 

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_scanf_nonzero_width : Warning<
   "zero field width in scanf format string is unused">,
   InGroup<Format>;
 def warn_format_conversion_argument_type_mismatch : Warning<
   "format specifies type %0 but the argument has "
   "%select{type|underlying type}2 %1">,
   InGroup<Format>;
 def warn_format_conversion_argument_type_mismatch_pedantic : Extension<
-  "format specifies type %0 but the argument has "
-  "%select{type|underlying type}2 %1">,
+  warn_format_conversion_argument_type_mismatch.Text>,
   InGroup<FormatPedantic>;
+def warn_format_conversion_argument_type_mismatch_confusion : Warning<
+  warn_format_conversion_argument_type_mismatch.Text>,
+  InGroup<FormatTypeConfusion>, DefaultIgnore;
 def warn_format_argument_needs_cast : Warning<
   "%select{values of type|enum values with underlying type}2 '%0' should not "
   "be used as format arguments; add an explicit cast to %1 instead">,
   InGroup<Format>;
 def warn_format_argument_needs_cast_pedantic : Warning<
-  "%select{values of type|enum values with underlying type}2 '%0' should not "
-  "be used as format arguments; add an explicit cast to %1 instead">,
+  warn_format_argument_needs_cast.Text>,
   InGroup<FormatPedantic>, DefaultIgnore;
 def warn_printf_positional_arg_exceeds_data_args : Warning <
   "data argument position '%0' exceeds the number of data arguments (%1)">,
   InGroup<Format>;
 def warn_format_zero_positional_specifier : Warning<
   "position arguments in format strings start counting at 1 (not 0)">,
   InGroup<Format>;
 def warn_format_invalid_positional_specifier : Warning<

cfe/trunk/lib/AST/FormatString.cpp

case SpecificTy: {
           default:
             break;
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
           case BuiltinType::Char_U:
           case BuiltinType::UChar:
           case BuiltinType::Bool:
             if (T == C.UnsignedShortTy || T == C.ShortTy)
-              return NoMatchPedantic;
+              return NoMatchTypeConfusion;
             return T == C.UnsignedCharTy || T == C.SignedCharTy ? Match
                                                                 : NoMatch;
           case BuiltinType::Short:
             return T == C.UnsignedShortTy ? Match : NoMatch;
           case BuiltinType::UShort:
             return T == C.ShortTy ? Match : NoMatch;
           case BuiltinType::Int:
             return T == C.UnsignedIntTy ? Match : NoMatch;

cfe/trunk/lib/Sema/SemaChecking.cpp

if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::cArg &&
     llvm::raw_svector_ostream os(FSString);
     FS.toString(os);
     EmitFormatDiagnostic(S.PDiag(diag::warn_format_bool_as_character)
                              << FSString,
                          E->getExprLoc(), false, CSR);
     return true;
   }
 
-  const analyze_printf::ArgType::MatchKind Match =
-      AT.matchesType(S.Context, ExprTy);
-  bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
+  analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
   if (Match == analyze_printf::ArgType::Match)
     return true;
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
   // and function pointer decay (seeing that an argument intended to be a
   // string has type 'char [6]' is probably more confusing than 'char *') and
   // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
   if (const ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
     if (isArithmeticArgumentPromotion(S, ICE)) {
       E = ICE->getSubExpr();
       ExprTy = E->getType();
 
       // Check if we didn't match because of an implicit cast from a 'char'
       // or 'short' to an 'int'.  This is done because printf is a varargs
       // function.
       if (ICE->getType() == S.Context.IntTy ||
           ICE->getType() == S.Context.UnsignedIntTy) {
         // All further checking is done on the subexpression
         const analyze_printf::ArgType::MatchKind ImplicitMatch =
             AT.matchesType(S.Context, ExprTy);
         if (ImplicitMatch == analyze_printf::ArgType::Match)
           return true;
-        if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
-          Pedantic = true;
+        if (ImplicitMatch == ArgType::NoMatchPedantic ||
+            ImplicitMatch == ArgType::NoMatchTypeConfusion)
+          Match = ImplicitMatch;
       }
     }
   } else if (const CharacterLiteral *CL = dyn_cast<CharacterLiteral>(E)) {
     // Special case for 'a', which has type 'int' in C.
     // Note, however, that we do /not/ want to treat multibyte constants like
     // 'MooV' as characters! This form is deprecated but still exists.
     if (ExprTy == S.Context.IntTy)
       if (llvm::isUIntN(S.Context.getCharWidth(), CL->getValue()))
if (S.Context.getTargetInfo().getTriple().isOSDarwin()) {
     QualType CastTy;
     std::tie(CastTy, CastTyName) = shouldNotPrintDirectly(S.Context, IntendedTy, E);
     if (!CastTy.isNull()) {
       // %zi/%zu and %td/%tu are OK to use for NSInteger/NSUInteger of type int
       // (long in ASTContext). Only complain to pedants.
       if ((CastTyName == "NSInteger" || CastTyName == "NSUInteger") &&
           (AT.isSizeT() || AT.isPtrdiffT()) &&
           AT.matchesType(S.Context, CastTy))
-        Pedantic = true;
+        Match = ArgType::NoMatchPedantic;
       IntendedTy = CastTy;
       ShouldNotPrintDirectly = true;
     }
   }
 
   // We may be able to offer a FixItHint if it is a supported type.
   PrintfSpecifier fixedFS = FS;
   bool Success =
       fixedFS.fixType(IntendedTy, S.getLangOpts(), S.Context, isObjCContext());
 
   if (Success) {
     // Get the fix string from the fixed format specifier
     SmallString<16> buf;
     llvm::raw_svector_ostream os(buf);
     fixedFS.toString(os);
 
     CharSourceRange SpecRange = getSpecifierRange(StartSpecifier, SpecifierLen);
 
     if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
-      unsigned Diag =
-          Pedantic
-              ? diag::warn_format_conversion_argument_type_mismatch_pedantic
-              : diag::warn_format_conversion_argument_type_mismatch;
+      unsigned Diag;
+      switch (Match) {
+      case ArgType::Match: llvm_unreachable("expected non-matching");
+      case ArgType::NoMatchPedantic:
+        Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
+        break;
+      case ArgType::NoMatchTypeConfusion:
+        Diag = diag::warn_format_conversion_argument_type_mismatch_confusion;
+        break;
+      case ArgType::NoMatch:
+        Diag = diag::warn_format_conversion_argument_type_mismatch;
+        break;
+      }
+
       // In this case, the specifier is wrong and should be changed to match
       // the argument.
       EmitFormatDiagnostic(S.PDiag(Diag)
                                << AT.getRepresentativeTypeName(S.Context)
                                << IntendedTy << IsEnum << E->getSourceRange(),
                            E->getBeginLoc(),
                            /*IsStringLocation*/ false, SpecRange,
                            FixItHint::CreateReplacement(SpecRange, os.str()));
if (IntendedTy == ExprTy && !ShouldNotPrintDirectly) {
         // The expression has a type that should not be printed directly.
         // We extract the name from the typedef because we don't want to show
         // the underlying type in the diagnostic.
         StringRef Name;
         if (const TypedefType *TypedefTy = dyn_cast<TypedefType>(ExprTy))
           Name = TypedefTy->getDecl()->getName();
         else
           Name = CastTyName;
-        unsigned Diag = Pedantic
+        unsigned Diag = Match == ArgType::NoMatchPedantic
                             ? diag::warn_format_argument_needs_cast_pedantic
                             : diag::warn_format_argument_needs_cast;
         EmitFormatDiagnostic(S.PDiag(Diag) << Name << IntendedTy << IsEnum
                                            << E->getSourceRange(),
                              E->getBeginLoc(), /*IsStringLocation=*/false,
                              SpecRange, Hints);
       } else {
         // In this case, the expression could be printed using a different
if (Success) {
     const CharSourceRange &CSR = getSpecifierRange(StartSpecifier,
                                                    SpecifierLen);
     // Since the warning for passing non-POD types to variadic functions
     // was deferred until now, we emit a warning for non-POD
     // arguments here.
     switch (S.isValidVarArgType(ExprTy)) {
     case Sema::VAK_Valid:
     case Sema::VAK_ValidInCXX11: {
-      unsigned Diag =
-          Pedantic
-              ? diag::warn_format_conversion_argument_type_mismatch_pedantic
-              : diag::warn_format_conversion_argument_type_mismatch;
+      unsigned Diag;
+      switch (Match) {
+      case ArgType::Match: llvm_unreachable("expected non-matching");
+      case ArgType::NoMatchPedantic:
+        Diag = diag::warn_format_conversion_argument_type_mismatch_pedantic;
+        break;
+      case ArgType::NoMatchTypeConfusion:
+        Diag = diag::warn_format_conversion_argument_type_mismatch_confusion;
+        break;
+      case ArgType::NoMatch:
+        Diag = diag::warn_format_conversion_argument_type_mismatch;
+        break;
+      }
 
       EmitFormatDiagnostic(
           S.PDiag(Diag) << AT.getRepresentativeTypeName(S.Context) << ExprTy
                         << IsEnum << CSR << E->getSourceRange(),
           E->getBeginLoc(), /*IsStringLocation*/ false, CSR);
       break;
     }
     case Sema::VAK_Undefined:

cfe/trunk/test/Sema/format-bool.c

 // RUN: %clang_cc1 -xc %s -verify -DBOOL=_Bool
 // RUN: %clang_cc1 -xc++ %s -verify -DBOOL=bool
 // RUN: %clang_cc1 -xobjective-c %s -verify -DBOOL=_Bool
-// RUN: %clang_cc1 -xc %s -verify -DBOOL=_Bool -Wformat-pedantic -DPEDANTIC
-// RUN: %clang_cc1 -xc++ %s -verify -DBOOL=bool -Wformat-pedantic -DPEDANTIC
+// RUN: %clang_cc1 -xc %s -verify -DBOOL=_Bool -Wformat-type-confusion -DTYPE_CONF
+// RUN: %clang_cc1 -xc++ %s -verify -DBOOL=bool -Wformat-type-confusion -DTYPE_CONF
 
 __attribute__((format(__printf__, 1, 2)))
 int p(const char *fmt, ...);
 
 BOOL b;
 
 #ifdef __OBJC__
 @interface NSString
 +(NSString *)stringWithFormat:(NSString *)fmt, ...
     __attribute__((format(__NSString__, 1, 2)));
 @end
 
 #define YES __objc_yes
 #define NO __objc_no
 #endif
 
 int main() {
   p("%d", b);
   p("%hd", b);
-#ifdef PEDANTIC
+#ifdef TYPE_CONF
   // expected-warning@-2 {{format specifies type 'short' but the argument has type}}
 #endif
   p("%hhd", b);
   p("%u", b);
   p("%hu", b);
-#ifdef PEDANTIC
+#ifdef TYPE_CONF
   // expected-warning@-2 {{format specifies type 'unsigned short' but the argument has type}}
 #endif
   p("%hhu", b);
   p("%c", b); // expected-warning {{using '%c' format specifier, but argument has boolean value}}
   p("%lc", b); // expected-warning {{using '%lc' format specifier, but argument has boolean value}}
   p("%c", 1 == 1); // expected-warning {{using '%c' format specifier, but argument has boolean value}}
   p("%f", b); // expected-warning{{format specifies type 'double' but the argument has type}}
   p("%ld", b); // expected-warning{{format specifies type 'long' but the argument has type}}
   p("%lld", b); // expected-warning{{format specifies type 'long long' but the argument has type}}
 
 #ifdef __OBJC__
   [NSString stringWithFormat: @"%c", 0]; // probably fine?
   [NSString stringWithFormat: @"%c", NO]; // expected-warning {{using '%c' format specifier, but argument has boolean value}}
 #endif
 }

cfe/trunk/test/Sema/format-strings-pedantic.c

-// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem %S/Inputs %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
+// RUN: %clang_cc1 -xobjective-c -fblocks -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
+// RUN: %clang_cc1 -xc++ -fsyntax-only -verify -Wno-format -Wformat-pedantic %s
 
+__attribute__((format(printf, 1, 2)))
 int printf(const char *restrict, ...);
 
-typedef unsigned char uint8_t;
+int main() {
+  printf("%p", (int *)0); // expected-warning {{format specifies type 'void *' but the argument has type 'int *'}}
+  printf("%p", (void *)0);
 
-void print_char_as_short() {
-  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1);       // expected-warning{{format specifies type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned char')}}
+#ifdef __OBJC__
+  printf("%p", ^{}); // expected-warning {{format specifies type 'void *' but the argument has type 'void (^)(void)'}}
+  printf("%p", (id)0); // expected-warning {{format specifies type 'void *' but the argument has type 'id'}}
+#endif
+
+#ifdef __cplusplus
+  printf("%p", nullptr); // expected-warning {{format specifies type 'void *' but the argument has type 'nullptr_t'}}
+#endif
 }

cfe/trunk/test/Sema/format-type-confusion.c

+// RUN: %clang_cc1 -triple x86_64-apple-darwin9.0 -fsyntax-only -verify -Wno-format -Wformat-type-confusion %s
+
+__attribute__((format(__printf__, 1, 2)))
+int printf(const char *msg, ...);
+
+#define FMT "%hd %hu %d %u %hhd %hhu %c"
+
+int main() {
+  _Bool b = 0;
+  printf(FMT,
+         b, // expected-warning {{format specifies type 'short' but the argument has type '_Bool'}}
+         b, // expected-warning {{format specifies type 'unsigned short' but the argument has type '_Bool'}}
+         b, b, b, b, b);
+
+  unsigned char uc = 0;
+  printf(FMT,
+         uc, // expected-warning {{format specifies type 'short' but the argument has type 'unsigned char'}}
+         uc, // expected-warning {{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
+         uc, uc, uc, uc, uc);
+
+  signed char sc = 0;
+  printf(FMT,
+         sc, // expected-warning {{format specifies type 'short' but the argument has type 'signed char'}}
+         sc, // expected-warning {{format specifies type 'unsigned short' but the argument has type 'signed char'}}
+         sc, sc, sc, sc, sc);
+}