[PATCH] D112579: Allow non-variadic functions to be attributed with `__attribute__((format))`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: clang/test/SemaCXX/attr-format.cpp:76
+  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);
----------------
fcloutier wrote:
> aaron.ballman wrote:
> > This pointed out an interesting test case. What should the behavior be for:
> > ```
> > format("%p", 0);
> > ```
> > Because that sure feels like a more reasonable thing for someone to write expecting it to be treated as a null pointer constant.
> I think that the current behavior is the right one:
> 
> ```
> test.c:4:17: warning: format specifies type 'void *' but the argument has type 'int' [-Wformat]
>         printf("%p\n", 0);
>                 ~~     ^
>                 %d
> ```
> 
> The warning goes away if you use `(void *)0`, as expected. `__attribute__((format))` has no semantic meaning, so we can't (and shouldn't) infer that 0 is a pointer based on the usage of %p.
Ah, you know what, I've convinced myself I was wrong and you're right. C2x 7.22.6.1p9 gives the latest conversion rules here, and I think passing `0`, despite being the null pointer constant, is UB when the format specifier is `%p`. On targets where `int` and `void *` are the same width, this diagnostic feels rather pedantic. But on systems where those differ, it seems more important to issue the warning... so I think you're correct that we should leave this behavior alone.

Thanks for thinking it through with me. :-)


================
Comment at: clang/test/SemaCXX/attr-format.cpp:77-78
+  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}}
----------------
fcloutier wrote:
> aaron.ballman wrote:
> > This likely isn't specific to your changes, but the `%p` in these examples should be warning the user (a function or function pointer is not a pointer to void or a pointer to a character type, so that call is UB).
> This is already a -Wformat-pedantic warning, which IMO is the right warning group for it:
> 
> ```
> test.c:4:17: warning: format specifies type 'void *' but the argument has type 'int (*)()' [-Wformat-pedantic]
>         printf("%p\n", main);
>                 ~~     ^~~~
> 1 warning generated.
> ```
> 
> The relevant bit is clang/lib/AST/FormatString.cpp:
> 
> ```
>     case CPointerTy:
>       if (argTy->isVoidPointerType()) {
>         return Match;
>       } if (argTy->isPointerType() || argTy->isObjCObjectPointerType() ||
>             argTy->isBlockPointerType() || argTy->isNullPtrType()) {
>         return NoMatchPedantic;
>       } else {
>         return NoMatch;
>       }
> ```
Ah, good that we have it in a pedantic diagnostic. I agree, it is a pedantic one, I thought we were missing it entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112579