[webkit-dev] Reminder regarding formatting uint64_t

[Sam Weinig]

> On Feb 27, 2019, at 4:05 PM, Keith Rollin <krollin at apple.com> wrote:
> 
>> On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro <mcatanzaro at igalia.com> wrote:
>> Hi,
>> 
>> For the past several years, I've been regularly fixing -Wformat 
>> warnings that look like this:
>> 
>> ../../Source/WebKit/WebProcess/WebPage/WebPage.cpp:3148:46: warning: 
>> format ‘%llu’ expects argument of type ‘long long unsigned 
>> int’, but argument 6 has type ‘uint64_t’ {aka ‘long unsigned 
>> int’} [-Wformat=]
>>         RELEASE_LOG_ERROR(ProcessSuspension, "%p - WebPage 
>> (PageID=%llu) - LayerTreeFreezeReason::ProcessSuspended was set when 
>> removing LayerTreeFreezeReason::PageTransition; current reasons are %d",
>> 
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>             this, m_pageID, m_LayerTreeFreezeReasons.toRaw());
>>                   ~~~~~~~~
>> 
>> Problem is that uint64_t is long long unsigned int on Mac, but only 
>> long unsigned int on Linux. It can't be printed with %llu, so please 
>> use PRIu64 instead. E.g.:
>> 
>> LOG("%llu", pageID); // wrong
>> LOG("%" PRIu64, pageID); // correct
>> 
>> This is good to keep in mind for any sized integer types, but uint64_t 
>> in particular since this is what causes problems for us in practice.
> 
> 
> 
>> On Feb 27, 2019, at 14:47, Ryosuke Niwa <rniwa at webkit.org> wrote:
>> 
>> We should probably stop using these formatting strings in favor of makeString by making *LOG* functions to use makeString behind the scene.
> 
> Formatting strings are pretty much required for the RELEASE_LOG* macros. These macros require a string literal for the formatting information, so you can’t say something like:
> 
> RELEASE_LOG_ERROR(ProcessSuspension, makeString(this, " - WebPage (PageID = ", m_pageID, "), - LayerTreeFreezeReason::ProcessSuspended was set when removing LayerTreeFreezeReason::PageTransition; current reasons are ", m_LayerTreeFreezeReasons.toRaw()));
> 
> This would not result in a string literal being passed to RELEASE_LOG, and you’d get a compiler error.
> 
> You could technically get away with passing your created string along with a “%s” format specification:
> 
> RELEASE_LOG_ERROR(ProcessSuspension, "%s", makeString(this, " - WebPage (PageID = ", m_pageID, "), - LayerTreeFreezeReason::ProcessSuspended was set when removing LayerTreeFreezeReason::PageTransition; current reasons are ", m_LayerTreeFreezeReasons.toRaw()));
> 
> But this is not advised. The underlying Cocoa os_log facility is able to greatly compress the logs stored in memory and on disk, as well as get corresponding performance benefits, by taking advantage of the fact that the formatting string is a literal that can be found in the executable’s binary image. When you log with a particular formatting string, that string is not included in the log archive, but a reference to it is. In the case that Michael gives, a reference to the big formatting string is stored along with the pointer, the unsigned long long, and the integer. In the above example, a reference to “%s” is stored, along with the fully-formatted string. This latter approach takes up a lot more memory.
> 
> So go wild with the LOG* macros, but understand the restrictions with the RELEASE_LOG* macros.

Seems like a fun project would be to attempt to generate the format string literal at compile time based on the parameters passed.

- Sam