[Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan-Huckleberry]

Summary

Link: https://bugs.llvm.org/show_bug.cgi?id=41467

[Nathan-Huckleberry]

• Add comment in test

[nickdesaulniers]

LGTM thanks for the patch!

[aaron.ballman]

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

I could imagine "confusing user intent" being a valid reason why someone might want this warning, so we may want to default-off this diagnostic (because the code is safe) but still provide users with a way to enable it.

clang/test/Sema/format-strings-enum-fixed-type.cpp

82 ↗

(On Diff #214966)

This comment is now incorrect.

[Nathan-Huckleberry]

In D66186#1630427, @aaron.ballman wrote:

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

[lebedev.ri]

In D66186#1631921, @Nathan-Huckleberry wrote:

In D66186#1630427, @aaron.ballman wrote:

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

That explains what the implementation does, but does not attempt to answer the question *why* things are the way they are.

I read https://bugs.llvm.org/show_bug.cgi?id=41467#c4 as

• any narrowing is always diagnosed
• promotion to wider than int is diagnosed
• passthrough is not diagnosed
• promotion to something smaller than int is diagnosed (the current case)

I can interpret it as: we already know that

In D66186#1631921, @Nathan-Huckleberry wrote:

Since printf is a variadic function, integral argument types are promoted to int.

therefore why are you first implicitly promoting to int and then implicitly truncating?
Did you mean to print the original value? Did you mean to print int?

That doesn't sound too outlandish to me.

[aaron.ballman]

In D66186#1631921, @Nathan-Huckleberry wrote:

In D66186#1630427, @aaron.ballman wrote:

There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does.

Since printf is a variadic function, integral argument types are promoted to int. The warning code runs the matchesType check twice, once to check if the promoted type (int) is able to be printed with the format and once to check if the original type (char) is able to be printed with the format.

printf("%d", [char]) is caught by the first case
printf("%hhd", [char]) is caught by the second case.

printf("%hd", [char]) is a warning because an exception has not been made for that case.

This all makes sense as to how things work today, but I was more wondering why they worked that way in the first place. I'm especially interested to know whether this is diagnosed because it shows confusion of the user's intent, because that seems like a valuable behavior to retain (though perhaps it doesn't need to be default-on).

[lebedev.ri]

(just want to mark it as "unanswered questions")

[Nathan-Huckleberry]

As far as I can tell this case was just overlooked. The original commit adding this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only allows chars to int and chars to chars. Another commit ignores typing of chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I did not see anything related to this particular case in previous commits.

[aaron.ballman]

In D66186#1632440, @Nathan-Huckleberry wrote:

As far as I can tell this case was just overlooked. The original commit adding this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only allows chars to int and chars to chars. Another commit ignores typing of chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I did not see anything related to this particular case in previous commits.

Hmm, it looks like, at least from this review, someone thought the behavior was for demonstrating user intent: http://llvm.org/viewvc/llvm-project?view=revision&revision=157961.

I've convinced myself that -Wformat should disable that diagnostic by default, but there is utility in keeping it exposed through a different format warning flag. It seems like -Wformat-pedantic should still diagnose this case.

[Nathan-Huckleberry]

• Warn when -Wformat-pedantic is set

[Nathan-Huckleberry]

• Cleanup test

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

8080–8083 ↗

(On Diff #216702)

Match isn't used outside of this block later on, so i don't think you need *this* change.

8100–8107 ↗

(On Diff #216702)

Just add a new variable

// All further checking is done on the subexpression
analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, ExprTy);
if (Match2 == analyze_printf::ArgType::Match)
  return true;
Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;

[aaron.ballman]

clang/test/Sema/format-strings-enum-fixed-type.cpp

82 ↗

(On Diff #214966)

Not quite what I had in mind. I would remove the // no-warning comments that were added and instead change the comment on line 82 to say This is not correct, but it is safe. Only warned in pedantic mode because '%hd' shows intent. or something along those lines.

clang/test/Sema/format-strings.c

280–281 ↗

(On Diff #216702)

I'd drop the no-warning comments here, or say warning with -Wformat-pedantic only if you think it adds value.

[nickdesaulniers]

clang/lib/Sema/SemaChecking.cpp

8101 ↗

(On Diff #216702)

Maybe leave the top level Match const and just create a new one? It may be surprising if someone goes to reuse Match below not noticing that it may be modified here.

[Nathan-Huckleberry]

• Add variable for implicit match and fix comments

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

8100–8107 ↗

(On Diff #216702)

Early return would simplify this still

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

8106 ↗

(On Diff #216713)

I don't think this needs to use |= true. If Pedantic was true, this is a noop. If it was false, this sets it to true. Either way the value is true, so I think it should just be Pedantic = true; The logic gets easier if you write this as:

if (ImplicitMatch && ImplicitMatch != analyze_printf::ArgType::NoMatchPedantic)
  return true;
Pedantic |= ImplicitMatch;

[Nathan-Huckleberry]

clang/lib/Sema/SemaChecking.cpp

8100–8107 ↗

(On Diff #216702)

Could be ArgType::NoMatch and wouldn't display a warning

[Nathan-Huckleberry]

clang/lib/Sema/SemaChecking.cpp

8100–8107 ↗

(On Diff #216702)

Wait this should definitely be an else-if

[Nathan-Huckleberry]

clang/lib/Sema/SemaChecking.cpp

8100–8107 ↗

(On Diff #216702)

Nevermind you are correct. Will remove else case.

[Nathan-Huckleberry]

• Remove else and |=

[lebedev.ri]

Please can you explain why the snippet i posted in-line does not work for you?

clang/lib/Sema/SemaChecking.cpp

8105–8107 ↗

(On Diff #216729)

I do not understand this.
Even if you set Pedantic = true, you will immediately return true without ever using that Pedantic.
This seems broken?

[Nathan-Huckleberry]

• Fix broken logic from previous revision

[nickdesaulniers]

clang/lib/Sema/SemaChecking.cpp

8107 ↗

(On Diff #216730)

At this point ImplicitMatch can only have the value analyze_printf::ArgType::NoMatchPedantic as ArgType::MatchKind is an enum with only 3 values (see clang/include/clang/AST/FormatString.h).

Rather than have conditional/ternaries for each case, I think 2 conditionals are maybe simpler:

if (ImplicitMatch == analyze_printf::ArgType::Match)
  return true;
else if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
  Pedantic = true;

The |= is kind of more complicated than simple assignment.

Then you don't need a block for if (ImplicitMatch).

[Nathan-Huckleberry]

• Simplify logic for readability

[nickdesaulniers]

Thanks for being responsive to all this code review! 💃🏽

[lebedev.ri]

Looks about right

clang/lib/Sema/SemaChecking.cpp

8105–8106 ↗

(On Diff #216739)

No else after return.

[aaron.ballman]

LGTM aside from the else after return nit.

[lebedev.ri]

In D66186#1642607, @aaron.ballman wrote:

LGTM aside from the else after return nit.

[Nathan-Huckleberry]

• Remove else

[lebedev.ri]

clang/lib/Sema/SemaChecking.cpp

8106 ↗

(On Diff #216884)

if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic) should have stayed probably?

[Nathan-Huckleberry]

• Add if without else

[lebedev.ri]

LG, thank you.

cfe/trunk/lib/AST/FormatString.cpp

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:
+            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;
           case BuiltinType::Int:
             return T == C.UnsignedIntTy ? Match : NoMatch;

cfe/trunk/lib/Sema/SemaChecking.cpp

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.
-        if (AT.matchesType(S.Context, ExprTy))
+        // 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;
       }
     }
   } 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()))

cfe/trunk/test/FixIt/format.m

void test_percent_C() {
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%d"
   // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)("
   // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:42-[[@LINE-3]]:42}:")"
 
   NSLog(@"%C", 0.0); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'double'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
 }
 
 
 void testSignedness(long i, unsigned long u) {

cfe/trunk/test/Sema/format-strings-enum-fixed-type.cpp

 typedef enum : char { CharConstant = 'a' } CharEnum;
 
 // %hhd is deliberately not required to be signed, because 'char' isn't either.
 // This is a separate code path in FormatString.cpp.
 void testChar(CharEnum input) {
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because '%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
   
   printf("%lld", input); // expected-warning{{format specifies type 'long long' but the argument has underlying type 'char'}}
   printf("%lld", CharConstant); // expected-warning{{format specifies type 'long long'}}
 }

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

+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem %S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+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')}}
+}

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

void test10(int x, float f, int i, long long lli) {
   printf("%c\n", 1.23); // expected-warning{{format specifies type 'int' but the argument has type 'double'}}
   printf("Format %d, is %! %f", 1, 4.4); // expected-warning{{invalid conversion specifier '!'}}
 }
 
 typedef unsigned char uint8_t;
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 'unsigned char' but the argument has type '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'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);       // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
   printf("%p", p); // no-warning
   printf("%p", 123); // expected-warning{{format specifies type 'void *' but the argument has type 'int'}}
   printf("%.4p", p); // expected-warning{{precision used with 'p' conversion specifier, resulting in undefined behavior}}
   printf("%+p", p); // expected-warning{{flag '+' results in undefined behavior with 'p' conversion specifier}}
   printf("% p", p); // expected-warning{{flag ' ' results in undefined behavior with 'p' conversion specifier}}