[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D131057#3697265 , @enh wrote:

>> I think making scanf in the same patch makes sense. Let me check existing 
>> tests...
>
> fwiw, most of the libcs seem to have been doing these separately because 
> scanf is harder. i hope to have bionic's scanf done this week though.
>
> (note in case you haven't already that there is no %B for scanf, just %b...)

Whoops, you're right, that recommended practice I saw nearby was for fwprintf 
and not fscanf.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

> I think making scanf in the same patch makes sense. Let me check existing 
> tests...

fwiw, most of the libcs seem to have been doing these separately because scanf 
is harder. i hope to have bionic's scanf done this week though.

(note in case you haven't already that there is no %B for scanf, just %b...)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D131057#3697165 , @aaron.ballman 
wrote:

> Thanks for working on this! Please also add a release note for it.

Will add:)

> I think there are changes missing for ScanfFormatString.cpp to handle `bArg` 
> and `BArg` that should be handled at the same time. WDYT?

I noticed this: 
https://github.com/llvm/llvm-project/issues/56885#issuecomment-1203636181

I think making scanf in the same patch makes sense. Let me check existing 
tests...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for working on this! Please also add a release note for it.

I think there are changes missing for ScanfFormatString.cpp to handle `bArg` 
and `BArg` that should be handled at the same time. WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Elliott Hughes via Phabricator via cfe-commits]

enh accepted this revision.
enh added a comment.

thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Dimitry Andric via Phabricator via cfe-commits]

dim added a comment.

> GCC 12 -Wformat -pedantic emits a warning:
>
>   warning: ISO C17 does not support the ‘%b’ gnu_printf format [-Wformat=]
>
> The behavior is not ported (and it's unclear to me how to implement it).

If you really want this, I think it can be implemented by looking at 
`LangOpts::LangStd`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Dimitry Andric via Phabricator via cfe-commits]

dim accepted this revision.
dim added a subscriber: emaste.
dim added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131057/new/

https://reviews.llvm.org/D131057

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: aaron.ballman, dim, enh, erik.pilkington.
Herald added a subscriber: StephenFan.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Close #56885. %b %B are from WG14 N2630 and supported by glibc from 2.35 
onwards and latest Android bionic.

Like GCC, we don't test library support.

GCC 12 -Wformat -pedantic emits a warning:

> warning: ISO C17 does not support the ‘%b’ gnu_printf format [-Wformat=]

The behavior is not ported (and it's unclear to me how to implement it).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131057

Files:
  clang/include/clang/AST/FormatString.h
  clang/lib/AST/FormatString.cpp
  clang/lib/AST/PrintfFormatString.cpp
  clang/test/Sema/format-strings-fixit.c
  clang/test/Sema/format-strings.c
  clang/test/SemaObjC/format-strings-objc.m

Index: clang/test/SemaObjC/format-strings-objc.m
===
--- clang/test/SemaObjC/format-strings-objc.m
+++ clang/test/SemaObjC/format-strings-objc.m
@@ -56,7 +56,7 @@
 
 void check_nslog(unsigned k) {
   NSLog(@"%d%%", k); // no-warning
-  NSLog(@"%s%lb%d", "unix", 10, 20); // expected-warning {{invalid conversion specifier 'b'}} expected-warning {{data argument not used by format string}}
+  NSLog(@"%s%lv%d", "unix", 10, 20); // expected-warning {{invalid conversion specifier 'v'}} expected-warning {{data argument not used by format string}}
 }
 
 // Check type validation
Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -199,7 +199,7 @@
 
 void check_invalid_specifier(FILE* fp, char *buf)
 {
-  printf("%s%lb%d","unix",10,20); // expected-warning {{invalid conversion specifier 'b'}} expected-warning {{data argument not used by format string}}
+  printf("%s%lv%d","unix",10,20); // expected-warning {{invalid conversion specifier 'v'}} expected-warning {{data argument not used by format string}}
   fprintf(fp,"%%%l"); // expected-warning {{incomplete format specifier}}
   sprintf(buf,"%ld%d%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}}
   snprintf(buf, 2, "%ld%;%d", 1, 2, 3); // expected-warning{{format specifies type 'long' but the argument has type 'int'}} expected-warning {{invalid conversion specifier ';'}} expected-warning {{data argument not used by format string}}
@@ -304,6 +304,7 @@
   printf("%qp", (void *)0); // expected-warning{{length modifier 'q' results in undefined behavior or no effect with 'p' conversion specifier}}
   printf("hhX %hhX", (unsigned char)10); // no-warning
   printf("llX %llX", (long long) 10); // no-warning
+  printf("%llb %llB", (long long) 10, (long long) 10); // no-warning
   // This is fine, because there is an implicit conversion to an int.
   printf("%d", (unsigned char) 10); // no-warning
   printf("%d", (long long) 10); // expected-warning{{format specifies type 'int' but the argument has type 'long long'}}
@@ -429,6 +430,7 @@
   // Bad flag usage
   printf("%#p", (void *) 0); // expected-warning{{flag '#' results in undefined behavior with 'p' conversion specifier}}
   printf("%0d", -1); // no-warning
+  printf("%0b%0B", -1u, -1u); // no-warning
   printf("%-p", (void *) 0); // no-warning
 #if !defined(__ANDROID__) && !defined(__Fuchsia__)
   printf("%#n", (int *) 0); // expected-warning{{flag '#' results in undefined behavior with 'n' conversion specifier}}
@@ -499,6 +501,7 @@
 void pr8641(void) {
   printf("%#x\n", 10);
   printf("%#X\n", 10);
+  printf("%#b %#B\n", 10, 10u);
 }
 
 void posix_extensions(void) {
@@ -508,6 +511,8 @@
   printf("%'i\n", 123456789); // no-warning
   printf("%'f\n", (float) 1.0); // no-warning
   printf("%'p\n", (void*) 0); // expected-warning{{results in undefined behavior with 'p' conversion specifier}}
+  printf("%'b\n", 123456789); // expected-warning{{results in undefined behavior with 'b' conversion specifier}}
+  printf("%'B\n", 123456789); // expected-warning{{results in undefined behavior with 'B' conversion specifier}}
 }
 
 // PR8486
@@ -540,6 +545,7 @@
   printf("%hhi", x); // no-warning
   printf("%c", x); // no-warning
   printf("%hhu", y); // no-warning
+  printf("%hhb %hhB", x, x); // no-warning
 }
 
 // Test suppression of individual warnings.
Index: clang/test/Sema/format-strings-fixit.c
===
--- clang/test/Sema/format-strings-fixit.c
+++ clang/test/Sema/format-strings-fixit.c
@@ -21,6 +21,7 @@
   printf("%s", (int) 123);
   printf("abc%0f", "testing testing 123");
   printf("%u", (long) -12);
+  printf("%b", (long) -13);
   printf("%p", 123);
   printf("%c\n", "x");
   printf("%c\n", 1.23);
@@ -179,6 +180,7 @@
 // CHECK: printf("%d", (int) 123);
 // CHECK: printf("abc%s", "testing