Re: [webkit-dev] Reminder regarding formatting uint64_t

[Ryosuke Niwa]

On Thu, Feb 28, 2019 at 12:40 PM Sam Weinig  wrote:

>
>
> > On Feb 27, 2019, at 4:05 PM, Keith Rollin  wrote:
> >
> >> 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  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.
>

Hm... os_log_* themselves seem to be macros:
https://developer.apple.com/documentation/os/os_log_debug?language=objc

I wonder if we can still use templates to detect types & generate the
static string though... Assuming, we can iterate over __VA_ARGS__ to
generate something like:
os_log_(ChannelStuff™, GENERATE_FORMAT_STRING(__VA_ARGS__), __VA_ARGS__)
and have GENERATE_FORMAT_STRING generate a sequence of formatString<>(_1) +
formatString<>(_2) + formatString<>(_3) + ...
where formatString generates static string which knows how to
concatenate itself using techniques like these:
https://akrzemi1.wordpress.com/2017/06/28/compile-time-string-concatenation/
https://stackoverflow.com/questions/15858141/conveniently-declaring-compile-time-strings-in-c
https://stackoverflow.com/questions/24783400/concatenate-compile-time-strings-in-a-template-at-compile-time

It's getting a bit too late for me to think straight though...

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Sam Weinig]

> On Feb 27, 2019, at 6:46 PM, Michael Catanzaro  wrote:
> 
> On Wed, Feb 27, 2019 at 6:05 PM, Keith Rollin  wrote:
>> The underlying Cocoa os_log facility
> 
> Yeah this is really interesting too. RELEASE_LOG is Cocoa-specific, because 
> it's only backed by os_log. So when you change debug LOGs to RELEASE_LOG, 
> you're removing the logging for other platforms. I wonder if we should change 
> that.

This seems really unfortunate. It seems like RELEASE_LOG should use whatever 
the platform native logging mechanism is, and only depend on os_log where 
available. I filed https://bugs.webkit.org/show_bug.cgi?id=195182 to track this.

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

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Sam Weinig]

> On Feb 27, 2019, at 4:05 PM, Keith Rollin  wrote:
> 
>> On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro  
>> 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  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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Michael Catanzaro]

On Wed, Feb 27, 2019 at 6:05 PM, Keith Rollin  wrote:

The underlying Cocoa os_log facility


Yeah this is really interesting too. RELEASE_LOG is Cocoa-specific, 
because it's only backed by os_log. So when you change debug LOGs to 
RELEASE_LOG, you're removing the logging for other platforms. I wonder 
if we should change that.


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.


Wow.

Michael

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

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Keith Rollin]

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

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Simon Fraser]

Or use LOG_WITH_STREAM() where I presume << pageID() << does the right thing.

We’d need some new RELEASE_LOG_ macros for that.

Simon

> On Feb 27, 2019, at 2:47 PM, Ryosuke Niwa  wrote:
> 
> We should probably stop using these formatting strings in favor of makeString 
> by making *LOG* functions to use makeString behind the scene.
> 
> See https://trac.webkit.org/changeset/242014 
>  for example.
> 
> - R. Niwa
> 
> On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro  > 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.
> 
> Thanks,
> 
> Michael
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org 
> https://lists.webkit.org/mailman/listinfo/webkit-dev 
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

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

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Adrian Perez de Castro]

Hello,

On a related note...

On Wed, 27 Feb 2019 16:35:39 -0600, Michael Catanzaro  
wrote:
 
> 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.

Related tip: if you want to print a “size_t” or “ssize_t”, use the
corresponding format specifiers “%zu” and “%zs” — do not cast the value
to some “big enough” integral type. Examples:

  size_t byteSize = /* ... */;
  LOG("size: %llu bytes", static_cast(byteSize)); // meh
  LOG("size: %zu bytes", byteSize); // better!

This is not as much of an issue as the issue with “uint64_t” mentioned by
Michael, but still nice to keep in mind as well.

Cheers,


-Adrián


pgpq5ut2IWFsf.pgp
Description: PGP signature
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Reminder regarding formatting uint64_t

[Ryosuke Niwa]

We should probably stop using these formatting strings in favor of
makeString by making *LOG* functions to use makeString behind the scene.

See https://trac.webkit.org/changeset/242014 for example.

- R. Niwa

On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro 
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.
>
> Thanks,
>
> Michael
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev