[Sema] Warning for _Float16 passed to format specifier '%f' #74439

Luhaocong · 2023-12-05T09:30:18Z

According to https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2844.pdf, default argument promotions for _FloatN types has been removed.

A warning is needed to notice user to promote _Float16 to double explicitly, and then pass it to format specifier '%f', which is consistent with GCC.

Fixes: #68538

llvmbot · 2023-12-05T09:30:51Z

@llvm/pr-subscribers-clang

Author: Haocong Lu (Luhaocong)

Changes

According to https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2844.pdf, default argument promotions for _FloatN types has been removed.

A warning is needed to notice user to promote _Float16 to double explicitly, and then pass it to format specifier '%f', which is consistent with GCC.

Fixes: #68538

Full diff: https://github.com/llvm/llvm-project/pull/74439.diff

5 Files Affected:

(modified) clang/lib/AST/FormatString.cpp (-1)
(modified) clang/test/AST/variadic-promotion.c (+5)
(modified) clang/test/Sema/attr-format.c (+7)
(modified) clang/test/SemaCXX/attr-format.cpp (+1)
(modified) clang/test/SemaCXX/format-strings-scanf.cpp (+10-6)

diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a2..c5d14b4af7ff1 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -488,7 +488,6 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
                 return NoMatchPromotionTypeConfusion;
               break;
             case BuiltinType::Half:
-            case BuiltinType::Float16:
             case BuiltinType::Float:
               if (T == C.DoubleTy)
                 return MatchPromotion;
diff --git a/clang/test/AST/variadic-promotion.c b/clang/test/AST/variadic-promotion.c
index 41c7fec8d7943..7cbadb832ca80 100644
--- a/clang/test/AST/variadic-promotion.c
+++ b/clang/test/AST/variadic-promotion.c
@@ -18,3 +18,8 @@ void test_floating_promotion(__fp16 *f16, float f32, double f64) {
 // CHECK: ImplicitCastExpr {{.*}} 'double' <FloatingCast>
 // CHECK-NEXT: 'float'
 }
+
+void test_Float16_no_default_promotion(_Float16 f16) {
+  variadic(1, f16);
+// CHECK-NOT: ImplicitCastExpr {{.*}} 'double' <FloatingCast>
+}
diff --git a/clang/test/Sema/attr-format.c b/clang/test/Sema/attr-format.c
index 1f4c864d4f78b..bdfd8425c4e9a 100644
--- a/clang/test/Sema/attr-format.c
+++ b/clang/test/Sema/attr-format.c
@@ -16,6 +16,8 @@ typedef const char *xpto;
 void j(xpto c, va_list list) __attribute__((format(printf, 1, 0))); // no-error
 void k(xpto c) __attribute__((format(printf, 1, 0)));               // no-error
 
+void l(char *a, _Float16 b) __attribute__((format(printf, 1, 2))); // expected-warning {{GCC requires a function with the 'format' attribute to be variadic}}
+
 void y(char *str) __attribute__((format(strftime, 1, 0)));             // no-error
 void z(char *str, int c, ...) __attribute__((format(strftime, 1, 2))); // expected-error {{strftime format attribute requires 3rd parameter to be 0}}
 
@@ -93,6 +95,11 @@ void call_nonvariadic(void) {
   d3("%s", 123); // expected-warning{{format specifies type 'char *' but the argument has type 'int'}}
 }
 
+void call_no_default_promotion(void) {
+  a("%f", (_Float16)1.0); // expected-warning{{format specifies type 'double' but the argument has type '_Float16'}}
+  l("%f", (_Float16)1.0); // expected-warning{{format specifies type 'double' but the argument has type '_Float16'}}
+}
+
 __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);
diff --git a/clang/test/SemaCXX/attr-format.cpp b/clang/test/SemaCXX/attr-format.cpp
index adc05fc46776c..4509c3a95e8ef 100644
--- a/clang/test/SemaCXX/attr-format.cpp
+++ b/clang/test/SemaCXX/attr-format.cpp
@@ -81,6 +81,7 @@ void do_format() {
 
   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("%f", (_Float16)123.f);// expected-warning{{format specifies type 'double' but the argument has type '_Float16'}}
   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);
diff --git a/clang/test/SemaCXX/format-strings-scanf.cpp b/clang/test/SemaCXX/format-strings-scanf.cpp
index 25fe5346791a0..406c2069e28ca 100644
--- a/clang/test/SemaCXX/format-strings-scanf.cpp
+++ b/clang/test/SemaCXX/format-strings-scanf.cpp
@@ -22,6 +22,7 @@ union bag {
     unsigned long long ull;
     signed long long sll;
     __fp16 f16;
+    _Float16 Float16;
     float ff;
     double fd;
     long double fl;
@@ -51,18 +52,21 @@ void test(void) {
     // 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@+4{{format specifies type 'float *' but the argument has type '__fp16 *'}}
+    // expected-warning@+3{{format specifies type 'float *' but the argument has type '_Float16 *'}}
     // 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);
+    scan("%f %f %f %f", &b.f16, &b.Float16, &b.fd, &b.fl);
 
-    // expected-warning@+3{{format specifies type 'double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+4{{format specifies type 'double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+3{{format specifies type 'double *' but the argument has type '_Float16 *'}}
     // 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);
+    scan("%lf %lf %lf %lf", &b.f16, &b.Float16, &b.ff, &b.fl);
 
-    // expected-warning@+3{{format specifies type 'long double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+4{{format specifies type 'long double *' but the argument has type '__fp16 *'}}
+    // expected-warning@+3{{format specifies type 'long double *' but the argument has type '_Float16 *'}}
     // 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);
+    scan("%Lf %Lf %Lf %Lf", &b.f16, &b.Float16, &b.ff, &b.fd);
 }

Luhaocong · 2023-12-05T14:36:09Z

This patch improves implement 04e6178. Could you please help me review this patch @apple-fcloutier @AaronBallman

apple-fcloutier · 2023-12-05T17:30:29Z

Should this only apply in C23 mode? Standard behavior until C23 is that _Float16 promotes to double. What about C++?

Luhaocong · 2023-12-06T07:49:07Z

Should this only apply in C23 mode? Standard behavior until C23 is that _Float16 promotes to double. What about C++?

It seems clang never support default argument promotions for _Float16, as described in https://github.com/llvm/llvm-project/blob/main/clang/docs/LanguageExtensions.rst#half-precision-floating-point five years ago. Is there any demand to support this in old C/C++ standard？

And I try case with -std=c99 (https://godbolt.org/z/xqx8e6oh5) and -std=c++11 (https://godbolt.org/z/Mrn779Tdq) , GCC give a warning at both.

AaronBallman · 2023-12-06T16:35:41Z

Should this only apply in C23 mode? Standard behavior until C23 is that _Float16 promotes to double. What about C++?

_Float16 wasn't standardized until C23; before then it was part of TS 18661, but we never claimed conformance to that (that I'm aware of), so I think we have the wiggle room to be consistent across language modes. My bigger question is: do we codegen properly and what do C standard libraries expect? e.g., if we codegen such that we don't promote in older language modes, will this cause problems for runtime libraries?

CC @jcranmer-intel for additional opinions

jcranmer-intel · 2023-12-20T20:00:10Z

Should this only apply in C23 mode? Standard behavior until C23 is that _Float16 promotes to double. What about C++?

I can't find any reference in older versions of C or TS 18661-3 that suggests that _Float16 is promoted to double. The wording of 6.5.2.2 used to say

If the expression that denotes the called function has a type that does not include a prototype, the integer promotions are performed on each argument, and arguments that have type float are promoted to double. These are called the default argument promotions.

Which suggests that only float is promoted and that other floating point types are not promoted, and nothing in TS 18661-3 can be construed to suggest promotion either (notably, _Float32 doesn't promote to double, even though it's likely the same representation as float!).

apple-fcloutier · 2023-12-20T20:26:38Z

N2844, as linked by OP, references the removed language that you are looking for.

As Aaron said, it doesn't matter if clang never claimed conformance/never implemented default argument promotion for these types in the first place.

jcranmer-intel · 2023-12-20T21:48:03Z

N2844, as linked by OP, references the removed language that you are looking for.

Doing some more spelunking, no released version of the TS ever had default argument promotion. The change to the TS was done shortly before integration in C, in response to the C++ proposal for extended floating-point types, but WG14 objected to the change, so it was dropped in actual integration. See https://mailman.oakapple.net/pipermail/cfp-interest/2020-September/001782.html for the summary.

Luhaocong · 2024-01-04T10:02:02Z

Thanks for everyone's review. Do you have any other suggestions for this change, and can this pull request be accepted ?

cor3ntin

LGTM.

The format diagnostics are new in Clang 18 and as such we do not need a release note. Correct?

cor3ntin · 2024-01-04T15:00:00Z

clang/test/AST/variadic-promotion.c

+void test_Float16_no_default_promotion(_Float16 f16) {
+  variadic(1, f16);
+// CHECK-NOT: ImplicitCastExpr {{.*}} 'double' <FloatingCast>
+}


This test seems unrelated to the change, we should do that in a separate PR

According to https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2844.pdf, there are no default argument promotions for _FloatN types. A warning is needed to notice user to promote _Float16 to double explicitly, and then pass it to format specifier '%f', which is consistent with GCC.

Luhaocong · 2024-01-05T06:33:22Z

LGTM.

The format diagnostics are new in Clang 18 and as such we do not need a release note. Correct?

These format diagnostics are existent until 04e6178 was committed. Clang 17.0.1 gives a warning: https://godbolt.org/z/xMdK9cETY.

cor3ntin

Thanks for confirming.
Do you need me to merge that for you?

Luhaocong · 2024-01-05T07:56:10Z

Thanks for confirming. Do you need me to merge that for you?

It can't be better, thanks.

rorth · 2024-01-08T08:01:19Z

This patch broke the Solaris/sparcv9 buildbot: the new/changed tests FAIL with

  File /opt/llvm-buildbot/home/solaris11-sparcv9/clang-solaris11-sparcv9/llvm/clang/test/SemaCXX/format-strings-scanf.cpp Line 25: _Float16 is not supported on this target
1 error generated.

Fixes #74439 (comment)

According to https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2844.pdf, default argument promotions for _FloatN types has been removed. A warning is needed to notice user to promote _Float16 to double explicitly, and then pass it to format specifier '%f', which is consistent with GCC. Fixes: llvm#68538

Fixes llvm#74439 (comment)

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 5, 2023

Luhaocong force-pushed the warn-Float16-in-format branch from 10c48f5 to afb0d39 Compare December 5, 2023 09:47

Luhaocong requested review from apple-fcloutier and AaronBallman December 6, 2023 07:50

Luhaocong force-pushed the warn-Float16-in-format branch from afb0d39 to 47ec170 Compare December 6, 2023 09:26

Luhaocong requested a review from jcranmer-intel December 7, 2023 01:38

Luhaocong force-pushed the warn-Float16-in-format branch from 47ec170 to 899e18d Compare January 4, 2024 09:22

cor3ntin added the c23 label Jan 4, 2024

cor3ntin approved these changes Jan 4, 2024

View reviewed changes

Luhaocong force-pushed the warn-Float16-in-format branch from 899e18d to e86dc46 Compare January 5, 2024 01:50

cor3ntin approved these changes Jan 5, 2024

View reviewed changes

Luhaocong merged commit 5034994 into llvm:main Jan 8, 2024

Luhaocong added a commit that referenced this pull request Jan 8, 2024

[Sema][test] Split format attribute test cases for _Float16

442f67c

Fixes #74439 (comment)

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024

[Sema][test] Split format attribute test cases for _Float16

81a1024

Fixes llvm#74439 (comment)

[Sema] Warning for _Float16 passed to format specifier '%f' #74439

[Sema] Warning for _Float16 passed to format specifier '%f' #74439

Uh oh!

Conversation

Luhaocong commented Dec 5, 2023

Uh oh!

llvmbot commented Dec 5, 2023

Uh oh!

Luhaocong commented Dec 5, 2023

Uh oh!

apple-fcloutier commented Dec 5, 2023

Uh oh!

Luhaocong commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Dec 6, 2023

Uh oh!

jcranmer-intel commented Dec 20, 2023

Uh oh!

apple-fcloutier commented Dec 20, 2023

Uh oh!

jcranmer-intel commented Dec 20, 2023

Uh oh!

Luhaocong commented Jan 4, 2024

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

cor3ntin Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

Luhaocong Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

Luhaocong commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cor3ntin left a comment

Choose a reason for hiding this comment

Uh oh!

Luhaocong commented Jan 5, 2024

Uh oh!

rorth commented Jan 8, 2024

Uh oh!

Uh oh!

Luhaocong commented Dec 6, 2023 •

edited

Loading

Luhaocong commented Jan 5, 2024 •

edited

Loading