[webkit-dev] Reminder regarding formatting uint64_t

[Ryosuke Niwa]

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... 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<T> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20190315/bd01a144/attachment.html>