[Sema] tolerate more promotion matches in format string checking

[fcloutier]

Summary

It's been reported that when using attribute((format)) on non-variadic functions, certain values that normally get promoted when passed as variadic arguments now unconditionally emit a diagnostic:

c
void foo(const char *fmt, float f) __attribute__((format(printf, 1, 2)));
void bar(void) {
	foo("%g", 123.f);
	//   ^ format specifies type 'double' but the argument has type 'float'
}

This is normally not an issue because float values get promoted to doubles when passed as variadic arguments, but needless to say, variadic argument promotion does not apply to non-variadic arguments.

While this can be fixed by adjusting the prototype of foo, this is sometimes undesirable in C (for instance, if foo is ABI). In C++, using variadic templates, this might instead require call-site fixing, which is tedious and arguably needless work:

c++
template<typename... Args>
void foo(const char *fmt, Args &&...args) __attribute__((format(printf, 1, 2)));
void bar(void) {
	foo("%g", 123.f);
	//   ^ format specifies type 'double' but the argument has type 'float'
}

To address this issue, we teach FormatString about a few promotions that have always been around but that have never been exercised in the direction that FormatString checks for:

• char, unsigned char -> int, unsigned
• half, float16, float -> double

This addresses issue https://github.com/llvm/llvm-project/issues/59824.

[aaron.ballman]

Thank you for this! The changes should come with a release note (in clang/docs/ReleaseNotes.rst). Also, you should upload the patch with -U999 amount of context so that reviewers can see the bigger picture a bit more easily.

clang/lib/AST/FormatString.cpp

468–474

bool types?

480–481

Should these be checking for T == C.FloatTy to return NoMatchPromotionTypeConfusion?

[fcloutier]

Add release note, ensure bool as a formatted formal parameter is accepted.

clang/lib/AST/FormatString.cpp

480–481

I don't think it's necessary. T is the format specifier's expected type, and no format specifier expects a float (due to floating-point types being promoted to double by default argument promotion), so there's never a case where T is FloatTy.

[aaron.ballman]

I just realized that we may need some additional test coverage for scanf, as that interface also uses ArgType::matchesType(): https://github.com/llvm/llvm-project/blob/5686f06d7fc02b7e2ab1eceb56f3830b6fdf7301/clang/lib/AST/ScanfFormatString.cpp#L511 but I think that's the last thing remaining on the patch.

clang/lib/AST/FormatString.cpp

480–481

Ah good point!

[fcloutier]

It seems that clang allows char specifiers to match bool in scanf today, without my change (https://godbolt.org/z/e8PrjY65h). I think that this is a mistake, but that's almost certainly up for debate and I'd like to avoid scope creep on this change.

The new format-strings-scanf.cpp file covers the correct and "barely wrong" cases that I could think of.

[aaron.ballman]

In D158318#4615285, @fcloutier wrote:

It seems that clang allows char specifiers to match bool in scanf today, without my change (https://godbolt.org/z/e8PrjY65h). I think that this is a mistake, but that's almost certainly up for debate and I'd like to avoid scope creep on this change.

Agreed! Probably worth filing an issue over it so we don't lose track of it, so: https://github.com/llvm/llvm-project/issues/64987

The new format-strings-scanf.cpp file covers the correct and "barely wrong" cases that I could think of.

Thank you, LGTM!

[fcloutier]

Apologies, I landed the change but forgot to update the commit message to include the "Differential Revision:" link. -_- I'm closing this change and I'll update the GitHub issue, which is linked.

[fcloutier]

Commit is https://github.com/llvm/llvm-project/commit/04e6178ae932c9a1d939dcfe3ef1189f4bbb21aa.

clang/docs/ReleaseNotes.rst

 * ``-Woverriding-t-option`` is renamed to ``-Woverriding-option``.
 
 Removed Compiler Flags
 -------------------------
 
 Attribute Changes in Clang
 --------------------------
 
+- When a non-variadic function is decorated with the ``format`` attribute,
+  Clang now checks that the format string would match the function's parameters'
+  types after default argument promotion. As a result, it's no longer an
+  automatic diagnostic to use parameters of types that the format style
+  supports but that are never the result of default argument promotion, such as
+  ``float``. (`#59824: <https://github.com/llvm/llvm-project/issues/59824>`_)
+
 Improvements to Clang's diagnostics
 -----------------------------------
 - Clang constexpr evaluator now prints template arguments when displaying
   template-specialization function calls.
 - Clang contexpr evaluator now displays notes as well as an error when a constructor
   of a base class is not called in the constructor of its derived class.
 - Clang no longer emits ``-Wmissing-variable-declarations`` for variables declared
   with the ``register`` storage class.

clang/lib/AST/FormatString.cpp

case SpecificTy: {
               return Match;
             break;
           }
           // "Partially matched" because of promotions?
           if (!Ptr) {
             switch (BT->getKind()) {
             default:
               break;
+            case BuiltinType::Bool:
+              if (T == C.IntTy || T == C.UnsignedIntTy)
+                return MatchPromotion;
+              break;
             case BuiltinType::Int:
             case BuiltinType::UInt:
               if (T == C.SignedCharTy || T == C.UnsignedCharTy ||
                   T == C.ShortTy || T == C.UnsignedShortTy || T == C.WCharTy ||
                   T == C.WideCharTy)
                 return MatchPromotion;
               break;
+            case BuiltinType::Char_U:
+              if (T == C.UnsignedIntTy)
+                return MatchPromotion;
+              if (T == C.UnsignedShortTy)
+                return NoMatchPromotionTypeConfusion;
+              break;
+            case BuiltinType::Char_S:
+              if (T == C.IntTy)
+                return MatchPromotion;
+              if (T == C.ShortTy)
+                return NoMatchPromotionTypeConfusion;
+              break;
+            case BuiltinType::Half:
+            case BuiltinType::Float16:
+            case BuiltinType::Float:
+              if (T == C.DoubleTy)
+                return MatchPromotion;
+              break;
             case BuiltinType::Short:
             case BuiltinType::UShort:
               if (T == C.SignedCharTy || T == C.UnsignedCharTy)
                 return NoMatchPromotionTypeConfusion;
               break;
             case BuiltinType::WChar_U:
             case BuiltinType::WChar_S:
               if (T != C.WCharTy && T != C.WideCharTy)

clang/test/Sema/attr-format.c

 const char *foo3(const char *format) __attribute__((format_arg("foo"))); // expected-error{{'format_arg' attribute requires parameter 1 to be an integer constant}}
 
 void call_nonvariadic(void) {
   d3("%i", 123);
   d3("%d", 123);
   d3("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
 }
 
-__attribute__((format(printf, 1, 2))) void forward_fixed(const char *fmt, int i) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
-  forward_fixed(fmt, i);
-  a(fmt, i);
-}
-
-__attribute__((format(printf, 1, 2))) void forward_fixed_2(const char *fmt, int i, int j) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
-  forward_fixed_2(fmt, i, j);
-  a(fmt, i);
+__attribute__((format(printf, 1, 2)))
+void forward_fixed(const char *fmt, _Bool b, char i, short j, int k, float l, double m) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+  forward_fixed(fmt, b, i, j, k, l, m);
+  a(fmt, b, i, j, k, l, m);
 }
 

clang/test/SemaCXX/attr-format.cpp

 
 void format_invalid_nonpod(const char *fmt, struct foo f) // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
     __attribute__((format(printf, 1, 2)));
 
 void do_format() {
   int x = 123;
   int &y = x;
   const char *s = "world";
+  bool b = false;
   format("bare string");
   format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, &do_format);
   format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
   format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
 
+  format("%c %c %hhd %hd %d\n", (char)'a', 'a', 'a', (short)123, (int)123);
+  format("%f %f %f\n", (__fp16)123.f, 123.f, 123.);
+  format("%Lf", (__fp16)123.f); // expected-warning{{format specifies type 'long double' but the argument has type '__fp16'}}
+  format("%Lf", 123.f); // expected-warning{{format specifies type 'long double' but the argument has type 'float'}}
+  format("%hhi %hhu %hi %hu %i %u", b, b, b, b, b, b);
+  format("%li", b); // expected-warning{{format specifies type 'long' but the argument has type 'bool'}}
+
   struct foo f;
   format_invalid_nonpod("hello %i", f); // expected-warning{{format specifies type 'int' but the argument has type 'struct foo'}}
 
   f.format("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
   f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, &do_format);
   f.format("%s %s %u %d %i %p\n", "hello", s, 10u, x, y, do_format);
   f.format("bad format %s"); // expected-warning{{more '%' conversions than data arguments}}
 }

clang/test/SemaCXX/format-strings-scanf.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat-nonliteral -Wformat-non-iso -fblocks -std=c++11 %s
+
+__attribute__((format(scanf, 1, 2)))
+int scanf(const char *, ...);
+
+template<typename... Args>
+__attribute__((format(scanf, 1, 2)))
+int scan(const char *fmt, Args &&...args) { // expected-warning{{GCC requires a function with the 'format' attribute to be variadic}}
+    return scanf(fmt, args...);
+}
+
+union bag {
+    bool b;
+    unsigned char uc;
+    signed char sc;
+    unsigned short us;
+    signed short ss;
+    unsigned int ui;
+    signed int si;
+    unsigned long ul;
+    signed long sl;
+    unsigned long long ull;
+    signed long long sll;
+    __fp16 f16;
+    float ff;
+    double fd;
+    long double fl;
+};
+
+void test(void) {
+    bag b;
+    scan("%hhi %hhu %hhi %hhu", &b.sc, &b.uc, &b.b, &b.b);
+    scan("%hi %hu", &b.ss, &b.us);
+    scan("%i %u", &b.si, &b.ui);
+    scan("%li %lu", &b.sl, &b.ul);
+    scan("%lli %llu", &b.sll, &b.ull);
+    scan("%f", &b.ff);
+    scan("%lf", &b.fd);
+    scan("%Lf", &b.fl);
+
+    // expected-warning@+4{{format specifies type 'short *' but the argument has type 'signed char *'}}
+    // expected-warning@+3{{format specifies type 'unsigned short *' but the argument has type 'unsigned char *'}}
+    // expected-warning@+2{{format specifies type 'short *' but the argument has type 'bool *'}}
+    // expected-warning@+1{{format specifies type 'unsigned short *' but the argument has type 'bool *'}}
+    scan("%hi %hu %hi %hu", &b.sc, &b.uc, &b.b, &b.b);
+
+    // expected-warning@+3{{format specifies type 'long *' but the argument has type 'short *'}}
+    // expected-warning@+2{{format specifies type 'char *' but the argument has type 'short *'}}
+    // expected-warning@+1{{format specifies type 'int *' but the argument has type 'short *'}}
+    scan("%hhi %i %li", &b.ss, &b.ss, &b.ss);
+
+    // expected-warning@+3{{format specifies type 'float *' but the argument has type '__fp16 *'}}
+    // expected-warning@+2{{format specifies type 'float *' but the argument has type 'double *'}}
+    // expected-warning@+1{{format specifies type 'float *' but the argument has type 'long double *'}}
+    scan("%f %f %f", &b.f16, &b.fd, &b.fl);
+
+    // expected-warning@+3{{format specifies type 'double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+2{{format specifies type 'double *' but the argument has type 'float *'}}
+    // expected-warning@+1{{format specifies type 'double *' but the argument has type 'long double *'}}
+    scan("%lf %lf %lf", &b.f16, &b.ff, &b.fl);
+
+    // expected-warning@+3{{format specifies type 'long double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+2{{format specifies type 'long double *' but the argument has type 'float *'}}
+    // expected-warning@+1{{format specifies type 'long double *' but the argument has type 'double *'}}
+    scan("%Lf %Lf %Lf", &b.f16, &b.ff, &b.fd);
+}