> On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro <mcatanz...@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 <rn...@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.

— Keith

_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Reply via email to