[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 ↗

(On Diff #219598)

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

27

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?

38

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. :-)

44

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 ↗

(On Diff #219598)

Sure, copied that verbatim.

clang/test/Sema/format-bool.c

27

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/lib/AST/FormatString.cpp

case AnyCharTy: {
         switch (BT->getKind()) {
           default:
             break;
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
           case BuiltinType::UChar:
           case BuiltinType::Char_U:
             return Match;
+          case BuiltinType::Bool:
+            return NoMatchPedantic;
           }
       return NoMatch;
     }
 
     case SpecificTy: {
       if (const EnumType *ETy = argTy->getAs<EnumType>()) {
         // If the enum is incomplete we know nothing about the underlying type.
         // Assume that it's 'int'.
         if (!ETy->getDecl()->isComplete())
case SpecificTy: {
           case BuiltinType::Char_S:
           case BuiltinType::SChar:
           case BuiltinType::Char_U:
           case BuiltinType::UChar:
             if (T == C.UnsignedShortTy || T == C.ShortTy)
               return NoMatchPedantic;
             return T == C.UnsignedCharTy || T == C.SignedCharTy ? Match
                                                                 : NoMatch;
+          case BuiltinType::Bool:
+            if (T == C.UnsignedShortTy || T == C.ShortTy ||
+                T == C.UnsignedCharTy || T == C.SignedCharTy)
+              return NoMatchPedantic;
+            return 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;
           case BuiltinType::UInt:
             return T == C.IntTy ? Match : NoMatch;

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 -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;
+
+int main() {
+  p("%d", b);
+  p("%hd", b);
+  p("%hhd", b);
+#ifdef PEDANTIC
+  // expected-warning@-3 {{format specifies type 'short' but the argument has type}}
+  // expected-warning@-3 {{format specifies type 'char' but the argument has type}}
+#endif
+  p("%u", b);
+  p("%hu", b);
+  p("%hhu", b);
+#ifdef PEDANTIC
+  // expected-warning@-3 {{format specifies type 'unsigned short' but the argument has type}}
+  // expected-warning@-3 {{format specifies type 'unsigned char' but the argument has type}}
+#endif
+  p("%c", b);
+  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}}
+}