Bug 213204 - Consistently use WTF_ATTRIBUTE_PRINTF in Assertions.[cpp,h]
Summary: Consistently use WTF_ATTRIBUTE_PRINTF in Assertions.[cpp,h]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-15 12:02 PDT by Michael Catanzaro
Modified: 2020-06-15 14:58 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.26 KB, patch)
2020-06-15 12:04 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-06-15 12:02:25 PDT
r262831 removed the GCC pragma that was suppressing these warnings:

[70/1715] Building CXX object Source/WTF/wtf/CMakeFiles/WTF.dir/Assertions.cpp.o
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp: In function ‘void vprintf_stderr_with_prefix(const char*, const char*, __va_list_tag*)’:
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp:186:56: warning: function ‘void vprintf_stderr_with_prefix(const char*, const char*, __va_list_tag*)’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  186 |     vprintf_stderr_common(formatWithPrefix.data(), args);
      |                                                        ^
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp: In function ‘void vprintf_stderr_with_trailing_newline(const char*, __va_list_tag*)’:
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp:193:43: warning: function ‘void vprintf_stderr_with_trailing_newline(const char*, __va_list_tag*)’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  193 |         vprintf_stderr_common(format, args);
      |                                           ^
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp:202:57: warning: function ‘void vprintf_stderr_with_trailing_newline(const char*, __va_list_tag*)’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  202 |     vprintf_stderr_common(formatWithNewline.data(), args);
      |                                                         ^
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp: In function ‘void WTFLogVaList(WTFLogChannel*, const char*, __va_list_tag*)’:
/home/mcatanzaro/Projects/WebKit/Source/WTF/wtf/Assertions.cpp:435:74: warning: function ‘void WTFLogVaList(WTFLogChannel*, const char*, __va_list_tag*)’ might be a candidate for ‘gnu_printf’ format attribute [-Wsuggest-attribute=format]
  435 |     String loggingString = WTF::createWithFormatAndArguments(format, args);
      | 

Solution is to use WTF_ATTRIBUTE_PRINTF where recommended. According to https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes, the first argument to WTF_ATTRIBUTE_PRINTF should be the position of the format string itself (starting from 1), and the second argument should be the position of the ... parameter pack (or 0  for functions that take va_list).
Comment 1 Michael Catanzaro 2020-06-15 12:04:58 PDT
Created attachment 401919 [details]
Patch
Comment 2 Darin Adler 2020-06-15 12:31:08 PDT
Comment on attachment 401919 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=401919&action=review

I didn’t set the commit queue flag yet because I’d like EWS to finish first.

> Source/WTF/wtf/Assertions.cpp:175
>  ALLOW_NONLITERAL_FORMAT_BEGIN

I wonder if we still need this?
Comment 3 Michael Catanzaro 2020-06-15 14:33:54 PDT
Comment on attachment 401919 [details]
Patch

EWS are looking green....

(In reply to Darin Adler from comment #2)
> > Source/WTF/wtf/Assertions.cpp:175
> >  ALLOW_NONLITERAL_FORMAT_BEGIN
> 
> I wonder if we still need this?

I'll check. Guess: probably yes.
Comment 4 Michael Catanzaro 2020-06-15 14:48:45 PDT
(In reply to Michael Catanzaro from comment #3)
> I'll check. Guess: probably yes.

I was wrong, it doesn't seem to be needed with GCC 10, at least I don't see any new warnings. I can't vouch for other compilers, though.
Comment 5 EWS 2020-06-15 14:57:35 PDT
Committed r263062: <https://trac.webkit.org/changeset/263062>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401919 [details].
Comment 6 Radar WebKit Bug Importer 2020-06-15 14:58:20 PDT
<rdar://problem/64380332>