Closed Bug 1102586 Opened 5 years ago Closed 5 years ago

TraceLoggingGraph.cpp:107:61: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘size_t {aka long unsigned int}’ [-Wformat=]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Recently-introduced build warning, with up-to-date mozilla-inbound and gcc 4.8, on 64-bit Ubuntu 14.10:
{
js/src/vm/TraceLoggingGraph.cpp: In member function ‘uint32_t TraceLoggerGraphState::nextLoggerId()’:
js/src/vm/TraceLoggingGraph.cpp:107:61: warning: format ‘%d’ expects argument of type ‘int’, but argument 3 has type ‘size_t {aka long unsigned int}’ [-Wformat=]
                            numLoggers, numLoggers, numLoggers);
                                                              ^
}

Looks like this file (TraceLoggingGraph.cpp) was only just created, in bug 1072903; hence, marking as blocking that bug.
Flags: needinfo?(hv1989)
I think we should probably be using "%z" here -- that's the format string for size_t.

(I recall that this wasn't standard C++ at some point, but according to a comment on [1], it's standard as of C++1x, and it's possible/likely that our C++1x requirements mean we can rely on %z being available. Plus, a MXR search shows that we already use it elsewhere [2] so we can probably rely on it.)

[1] https://stackoverflow.com/questions/2524611/how-to-print-size-t-variable-portably
[2] https://mxr.mozilla.org/mozilla-central/search?string=%25z&find=.cpp%24
As far as I can tell from the Microsoft documentation and Visual Studio blog posts, Visual Studio 2013 still doesn't support %z (and I don't see it mentioned for the Visual Studio 2015 preview either). MSVC has '%I' instead: http://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx
(Sorry, I guess I meant "%zu" - u for unsigned, and z to indicate number-of-bytes-in-an-unsigned)

Well, we do seem to use %zu here at least:
>141     fprintf(stderr, "  file %s line %u offset %zu\n",
>142             script()->filename(), (unsigned) script()->lineno(),
>143             script()->pcToOffset(pc()));
https://mxr.mozilla.org/mozilla-central/source/js/src/jit/RematerializedFrame.cpp#141

...and pcToOffset there does indeed return size_t:
> 1066     size_t pcToOffset(const jsbytecode *pc) const {
https://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.h#1066

RematerializedFrame.cpp doesn't seem to be excluded from our MSVC build, so I *think* MSVC is seeing & correctly-handling this format string there.

If we're worried though, we can always just use "%u" and static_cast<unsigned>(mySizeTValue).
Fixed in the patch queue. Thanks for reporting!
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(hv1989)
Resolution: --- → FIXED
btw, MSVC because does not support %zu, the current approach for printf-ing size_t is to use MFBT's (nonstandard) PRIuSIZE format macro (from bug 1114724) in cross-platform code.
You need to log in before you can comment on or make changes to this bug.