Reminder regarding formatting uint64_t

# Michael Catanzaro (18 days ago)

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

Contact us to advertise here
# Ryosuke Niwa (18 days ago)

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

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

  • R. Niwa

On Wed, Feb 27, 2019 at 2:36 PM Michael Catanzaro <mcatanzaro at igalia.com>

wrote:

# Adrian Perez de Castro (18 days ago)

Hello,

On a related note...

On Wed, 27 Feb 2019 16:35:39 -0600, Michael Catanzaro <mcatanzaro at igalia.com> 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<unsigned long long>(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.

# Simon Fraser (18 days ago)

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

We’d need some new RELEASELOG macros for that.

Simon

# Keith Rollin (18 days ago)

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.

— Keith

# Michael Catanzaro (18 days ago)

On Wed, Feb 27, 2019 at 6:05 PM, Keith Rollin <krollin at apple.com> 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

# Sam Weinig (17 days ago)

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 Weinig (17 days ago)

On Feb 27, 2019, at 6:46 PM, Michael Catanzaro <mcatanzaro at igalia.com> wrote:

On Wed, Feb 27, 2019 at 6:05 PM, Keith Rollin <krollin at apple.com> 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

# Ryosuke Niwa (3 days ago)

On Thu, Feb 28, 2019 at 12:40 PM Sam Weinig <weinig at apple.com> wrote:

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.

Hm... oslog* themselves seem to be macros: 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: oslog(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<T> generates static string which knows how to

concatenate itself using techniques like these: akrzemi1.wordpress.com/2017/06/28/compile-time-string-concatenation, stackoverflow.com/questions/15858141/conveniently-declaring-compile-time-strings-in-c, 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

Want more features?

Request early access to our private beta of readable email premium.