[Sema] Suppress -Wformat diagnostics for bool types when printed using %hhd

[erik.pilkington]

Summary

Instead, emit them under -Wformat-pedantic. This is particularly important for us because we have a lot of code that uses %hhd to print BOOL values, since on macOS BOOL is a typedef for signed char, and we emit fix-its recommending %hhd to print it. Now that this code is compiled on a target where BOOL is a typedef for _Bool, we're getting these -Wformat warnings. These aren't all that useful.

rdar://problem/54579473 Clang should not warn when using %hhd or %hd format specifiers on BOOL values

[aaron.ballman]

I'm wondering whether this should even warn pedantically. There are no format specifiers that apply directly to a _Bool datatype, so the user is left making a choice as to what specifier fits best. I think hhd and hhu are both equally reasonable types, as are the d and u groups directly -- I don't think we should warn on either of those specifiers with a _Bool argument. I could maybe see a case for pedantically diagnosing h, l, and ll prefixes with _Bool because that seems a bit type-confused to me. c as a specifier seems weird (given that false values will potentially terminate the string suddenly depending on terminal behavior, IIRC), but lc seems like type confusion again.

[erik.pilkington]

In D66856#1648809, @aaron.ballman wrote:

I'm wondering whether this should even warn pedantically. There are no format specifiers that apply directly to a _Bool datatype, so the user is left making a choice as to what specifier fits best. I think hhd and hhu are both equally reasonable types, as are the d and u groups directly -- I don't think we should warn on either of those specifiers with a _Bool argument. I could maybe see a case for pedantically diagnosing h, l, and ll prefixes with _Bool because that seems a bit type-confused to me. c as a specifier seems weird (given that false values will potentially terminate the string suddenly depending on terminal behavior, IIRC), but lc seems like type confusion again.

Hmm... on second though I think the l and ll prefixes are worth -Wformat proper, since that can result in printing a half-initialized integer when the argument ends up on the stack (we generate movl $0,(%rsp) for the first stack argument). I think %c is probably also worth a -Wformat, it seems like it would almost certainly be a mistake. I suppose we could pedantically warn on %hd, but I'm not sure. Who is the audience for -Wformat-pedantic? Users that expect it to diagnose any pedantically-UB printf calls will already be disappointed, since the C standard seems to be quite a lot more restrictive then us (7.21.6.9p9 says: "If any argument is not the correct type for the corresponding conversion specification, the behavior is undefined." (I'm assuming "correct type" means "same type")). So I think we should just concern ourselves with calls that would lead to actual problems at runtime, or are likely programmer errors, and printf("%hd", (_Bool)0) doesn't seem to meet either of those bars.

[aaron.ballman]

In D66856#1649721, @erik.pilkington wrote:

In D66856#1648809, @aaron.ballman wrote:

I'm wondering whether this should even warn pedantically. There are no format specifiers that apply directly to a _Bool datatype, so the user is left making a choice as to what specifier fits best. I think hhd and hhu are both equally reasonable types, as are the d and u groups directly -- I don't think we should warn on either of those specifiers with a _Bool argument. I could maybe see a case for pedantically diagnosing h, l, and ll prefixes with _Bool because that seems a bit type-confused to me. c as a specifier seems weird (given that false values will potentially terminate the string suddenly depending on terminal behavior, IIRC), but lc seems like type confusion again.

Hmm... on second though I think the l and ll prefixes are worth -Wformat proper, since that can result in printing a half-initialized integer when the argument ends up on the stack (we generate movl $0,(%rsp) for the first stack argument).

Agreed.

I think %c is probably also worth a -Wformat, it seems like it would almost certainly be a mistake.

That's my feeling as well. At the very least, -Wformat-pedantic.

I suppose we could pedantically warn on %hd, but I'm not sure.

I think it makes sense to dos o.

Who is the audience for -Wformat-pedantic?

To me, the audience are people who want to catch semantically-benign-but-logically-inconsistent format strings. e.g., it's not wrong to pass 'c' to something formatted as %hd, but it smells fishy to do so.

Users that expect it to diagnose any pedantically-UB printf calls will already be disappointed, since the C standard seems to be quite a lot more restrictive then us (7.21.6.9p9 says: "If any argument is not the correct type for the corresponding conversion specification, the behavior is undefined." (I'm assuming "correct type" means "same type")). So I think we should just concern ourselves with calls that would lead to actual problems at runtime, or are likely programmer errors, and printf("%hd", (_Bool)0) doesn't seem to meet either of those bars.

To me, actual problems at runtime belong in -Wformat and logical inconsistencies that don't cause runtime problems belong in -Wformat-pedantic. However, I think it's a defect that the C standard has no clearly UB-free way to print a _Bool value. I will bring this up on the reflectors to ask if we can clarify the standard here.

[erik.pilkington]

Address review comments.

[aaron.ballman]

In D66856#1650803, @aaron.ballman wrote:

To me, actual problems at runtime belong in -Wformat and logical inconsistencies that don't cause runtime problems belong in -Wformat-pedantic. However, I think it's a defect that the C standard has no clearly UB-free way to print a _Bool value. I will bring this up on the reflectors to ask if we can clarify the standard here.

The reflector discussion is still happening and there are issues with ambiguities that we are pretty sure we want to correct. I've got a paper out that touches on some of this: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2420.pdf

clang/include/clang/Basic/DiagnosticSemaKinds.td

8123

How about: using '%0' format specifier, but argument has boolean value and then pass in the character specifier used?

clang/test/Sema/format-bool.c

26

Just an FYI (not related to your patch): it seems that at least some people think this should be diagnosed as something other than by -Wformat-pedantic. Their thinking is that -Wformat-pedantic is for things that are required to have a diagnostic according to the standard but are not sufficiently interesting to warn about by default. This particular case is not required to be warned on by the standard, so it's not really a "pedantic" warning. It sounds like there may be interest in having -Wformat-pedantic for that understanding of pedantic, and introduce something like -Wformat-type-mismatch for these other cases where there is type confusion but not sufficiently dangerous to warrant warning by default?

37

The diagnostic is both correct and incorrect. Maybe that's fine. However, == returns an int in C and int is reasonable to pass in to %c, so the diagnostic seems a bit strange when it claims the argument has a boolean value. Then again, the results really are boolean despite the type being an int.

I think I've convinced myself this is fine as-is. :-)

43

Seems correct to me.

[erik.pilkington]

Update the diagnostic text.

[erik.pilkington]

In D66856#1666616, @aaron.ballman wrote:

The reflector discussion is still happening and there are issues with ambiguities that we are pretty sure we want to correct. I've got a paper out that touches on some of this: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2420.pdf

Nice, thanks for digging into this!

clang/include/clang/Basic/DiagnosticSemaKinds.td

8123

Sure, copied that verbatim.

clang/test/Sema/format-bool.c

26

That seems like a good idea to me, I agree that "pedantic" in the context of warnings means "technically incorrect according to the standard, but not really a big deal", which isn't really what -Wformat-pedantic is doing right now.

[erik.pilkington]

Ping!

[aaron.ballman]

LGTM, thank you for this!

clang/include/clang/Basic/DiagnosticSemaKinds.td

   "object format flags cannot be used with '%0' conversion specifier">,
   InGroup<Format>;
 def warn_printf_invalid_objc_flag: Warning<
     "'%0' is not a valid object format flag">,
     InGroup<Format>;
 def warn_scanf_scanlist_incomplete : Warning<
   "no closing ']' for '%%[' in scanf format string">,
   InGroup<Format>;
+def warn_format_bool_as_character : Warning<
+  "format specifies a character but argument has boolean value">,
+  InGroup<Format>;
 def note_format_string_defined : Note<"format string is defined here">;
 def note_format_fix_specifier : Note<"did you mean to use '%0'?">;
 def note_printf_c_str: Note<"did you mean to call the %0 method?">;
 def note_format_security_fixit: Note<
   "treat the string as an argument to avoid this">;
 
 def warn_null_arg : Warning<
   "null passed to a callee that requires a non-null argument">,

clang/lib/AST/Expr.cpp

case BO_Assign:
       return BO->getRHS()->isKnownToHaveBooleanValue();
     }
   }
 
   if (const ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E))
     return CO->getTrueExpr()->isKnownToHaveBooleanValue() &&
            CO->getFalseExpr()->isKnownToHaveBooleanValue();
 
+  if (isa<ObjCBoolLiteralExpr>(E))
+    return true;
+
   return false;
 }
 
 // Amusing macro metaprogramming hack: check whether a class provides
 // a more specific implementation of getExprLoc().
 //
 // See also Stmt.cpp:{getBeginLoc(),getEndLoc()}.
 namespace {

clang/lib/AST/FormatString.cpp

case AnyCharTy: {
       if (const BuiltinType *BT = argTy->getAs<BuiltinType>())
         switch (BT->getKind()) {
           default:
             break;
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
           case BuiltinType::UChar:
           case BuiltinType::Char_U:
+          case BuiltinType::Bool:
             return Match;
         }
       return NoMatch;
     }
 
     case SpecificTy: {
       if (const EnumType *ETy = argTy->getAs<EnumType>()) {
         // If the enum is incomplete we know nothing about the underlying type.
case SpecificTy: {
       if (const BuiltinType *BT = argTy->getAs<BuiltinType>())
         switch (BT->getKind()) {
           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 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;

clang/lib/Sema/SemaChecking.cpp

   if (!AT.isValid())
     return true;
 
   QualType ExprTy = E->getType();
   while (const TypeOfExprType *TET = dyn_cast<TypeOfExprType>(ExprTy)) {
     ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
+  // Diagnose attempts to print a boolean value as a character. Unlike other
+  // -Wformat diagnostics, this is fine from a type perspective, but it still
+  // doesn't make sense.
+  if (FS.getConversionSpecifier().getKind() == ConversionSpecifier::cArg &&
+      E->isKnownToHaveBooleanValue()) {
+    const CharSourceRange &CSR =
+        getSpecifierRange(StartSpecifier, SpecifierLen);
+    SourceLocation ExprLoc = E->getExprLoc();
+    EmitFormatDiagnostic(S.PDiag(diag::warn_format_bool_as_character)
+                             << ExprLoc,
+                         ExprLoc, false, CSR);
+    return true;
+  }
+
   const analyze_printf::ArgType::MatchKind Match =
       AT.matchesType(S.Context, ExprTy);
   bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
   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

clang/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
+
+__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
+  // expected-warning@-2 {{format specifies type 'short' but the argument has type}}
+#endif
+  p("%hhd", b);
+  p("%u", b);
+  p("%hu", b);
+#ifdef PEDANTIC
+  // expected-warning@-2 {{format specifies type 'unsigned short' but the argument has type}}
+#endif
+  p("%hhu", b);
+  p("%c", b); // expected-warning {{format specifies a character but argument has boolean value}}
+  p("%lc", b); // expected-warning {{format specifies a character but argument has boolean value}}
+  p("%c", 1 == 1); // expected-warning {{format specifies a character 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 {{format specifies a character but argument has boolean value}}
+#endif
+}