[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–83

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

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

8100–8108

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–83

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–282

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

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–8108

Early return would simplify this still

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

8105

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–8108

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

[Nathan-Huckleberry]

clang/lib/Sema/SemaChecking.cpp

8100–8108

Wait this should definitely be an else-if

[Nathan-Huckleberry]

clang/lib/Sema/SemaChecking.cpp

8100–8108

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

8103–8105

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

8106

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

8100–8108

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

8101

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

[Nathan-Huckleberry]

• Add if without else

[lebedev.ri]

LG, thank you.

clang/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;

clang/lib/Sema/SemaChecking.cpp

CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
   if (!AT.isValid())
     return true;
 
   QualType ExprTy = E->getType();
   while (const TypeOfExprType *TET = dyn_cast<TypeOfExprType>(ExprTy)) {
     ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  const analyze_printf::ArgType::MatchKind Match =
-      AT.matchesType(S.Context, ExprTy);
+  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
   // 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.
-        if (AT.matchesType(S.Context, ExprTy))
+        // All further checking is done on the subexpression
+        Match = AT.matchesType(S.Context, ExprTy);
+        if (Match) {
+          if (Match == analyze_printf::ArgType::NoMatchPedantic)
+            Pedantic = true;
+          else
             return 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()))
         ExprTy = S.Context.CharTy;
   }

clang/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) {

clang/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'}}
-  
+  printf("%hd", input);        // no-warning
+  printf("%hd", CharConstant); // no-warning
   // 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'}}
 }

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

+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem %S/Inputs %s
+
+#include <stdarg.h>
+#include <stddef.h>
+#define __need_wint_t
+#include <stddef.h> // For wint_t and wchar_t
+
+typedef struct _FILE FILE;
+int fprintf(FILE *, const char *restrict, ...);
+int printf(const char *restrict, ...);
+int snprintf(char *restrict, size_t, const char *restrict, ...);
+int sprintf(char *restrict, const char *restrict, ...);
+int vasprintf(char **, const char *, va_list);
+int asprintf(char **, const char *, ...);
+int vfprintf(FILE *, const char *restrict, va_list);
+int vprintf(const char *restrict, va_list);
+int vsnprintf(char *, size_t, const char *, va_list);
+int vsprintf(char *restrict, const char *restrict, va_list);
+
+int vscanf(const char *restrict format, va_list arg);
+
+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' (aka 'unsigned char')}}
+}

clang/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); // no-warning
+  printf("%hu\n", (uint8_t)1);       // no-warning
 }
 
 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}}
   printf("%0p", p); // expected-warning{{flag '0' results in undefined behavior with 'p' conversion specifier}}