JSC disassembler warns about incorrect printf format on 64-bit Linux

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Brendan Long]

In A64DOpcode.h, we use '%llx' for uint64_t's. On 64-bit Linux, uint64_t is a long unsigned, not a long long unsigned. This may occur on other platforms.

I'm not sure if this actually matters, but I'm trying to get rid of build warnings in WebKitGTK.

The PRIx64 macro should expand to the correct format for each platform. I think it doesn't work on Visual Studio, but it's defined in Source/JavaScriptCore/os-win32/inttypes.h. I'm hoping the build will automatically pick that up, but I can add a special import for it if necessary.

Build output:

  CXX      Source/JavaScriptCore/disassembler/libjavascriptcoregtk_3_0_la-ARM64Disassembler.lo
In file included from ../../Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:27:0:
../../Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h: In member function 'void JSC::ARM64Disassembler::A64DOpcode::appendUnsignedImmediate64(uint64_t)':
../../Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h:175:42: error: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]
         bufferPrintf("#0x%llx", immediate);
                                          ^
../../Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h: In member function 'void JSC::ARM64Disassembler::A64DOpcode::appendPCRelativeOffset(uint32_t*, int32_t)':
../../Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h:180:74: error: format '%llx' expects argument of type 'long long unsigned int', but argument 3 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]
         bufferPrintf("0x%llx", reinterpret_cast<uint64_t>(pc + immediate));

[Brendan Long]

Created attachment 220171 [details]
Patch

[Brendan Long]

Does EWS not test Windows anymore? I really don't want to find a Windows computer to test this :\

[Brendan Long]

Created attachment 220173 [details]
Patch

[Brendan Long]

The gtk-wk2 failure is interesting, since it works on my machine :\ I'm trying adding #include <inttypes.h> to see if that's what it's angry about.

[Brendan Long]

Looks like I might need to setup a VM to figure out why EWS doesn't like this.

[kov's GTK+ EWS bot]

Comment on attachment 220173 [details]
Patch

Attachment 220173 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/4911444084654080

[Zan Dobersek]

The __STDC_FORMAT_MACROS macro define is required before the <inttypes.h> include for the PRIx64 macro to be usable in C++ code.
http://stackoverflow.com/questions/8132399/how-to-printf-uint64-t

[Brendan Long]

Created attachment 220264 [details]
Patch

[Brendan Long]

(In reply to comment #7)
> The __STDC_FORMAT_MACROS macro define is required before the <inttypes.h> include for the PRIx64 macro to be usable in C++ code.
> http://stackoverflow.com/questions/8132399/how-to-printf-uint64-t

It looks like gcc doesn't require that anymore, which is why it works on my machine (Fedora 20), but now EWS (Ubuntu 12.04?).

[EFL EWS Bot]

Comment on attachment 220264 [details]
Patch

Attachment 220264 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5151989633646592

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 220264 [details])
> Attachment 220264 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/5151989633646592

When I build this patch locally, there is no build break. Could you test this again ?

[Zan Dobersek]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 220264 [details] [details])
> > Attachment 220264 [details] [details] did not pass efl-wk2-ews (efl-wk2):
> > Output: http://webkit-queues.appspot.com/results/5151989633646592
> 
> When I build this patch locally, there is no build break. Could you test this again ?

I got a similar compiler crash in bug #126388 -- there's an issue with the EWS it seems, perhaps a corrupted ccache entry?

[Zan Dobersek]

(In reply to comment #9)
> (In reply to comment #7)
> > The __STDC_FORMAT_MACROS macro define is required before the <inttypes.h> include for the PRIx64 macro to be usable in C++ code.
> > http://stackoverflow.com/questions/8132399/how-to-printf-uint64-t
> 
> It looks like gcc doesn't require that anymore, which is why it works on my machine (Fedora 20), but now EWS (Ubuntu 12.04?).

What GCC/libstdc++ version are you using on your system?

[Brendan Long]

(In reply to comment #13)
> What GCC/libstdc++ version are you using on your system?

I'm on 4.8.2 (Ubuntu 12.04 uses 4.6.3 apparently).

[Brendan Long]

Created attachment 220302 [details]
Patch

[Brendan Long]

Now it passes (same patch though).

[Brendan Long]

Can someone review this so we can commit it?

[Philippe Normand]

Comment on attachment 220302 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220302&action=review

> Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:27
> +#define __STDC_FORMAT_MACROS

Is there a good reason to have this here instead of just before the inttypes.h include?

[Brendan Long]

(In reply to comment #18)
> (From update of attachment 220302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220302&action=review
> 
> > Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:27
> > +#define __STDC_FORMAT_MACROS
> 
> Is there a good reason to have this here instead of just before the inttypes.h include?

I thought the style checker would complain about that, but apparently not. I'll change it.

[Brendan Long]

(In reply to comment #18)
> (From update of attachment 220302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220302&action=review
> 
> > Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:27
> > +#define __STDC_FORMAT_MACROS
> 
> Is there a good reason to have this here instead of just before the inttypes.h include?

Oh, I see what you mean. It should be before inttypes.h, not before stdint.h. I'm not sure why putting it there worked, but I'll move it before inttypes.h, since that's where it's supposed to be.

[Brendan Long]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 220302 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220302&action=review
> > 
> > > Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.cpp:27
> > > +#define __STDC_FORMAT_MACROS
> > 
> > Is there a good reason to have this here instead of just before the inttypes.h include?
> 
> Oh, I see what you mean. It should be before inttypes.h, not before stdint.h. I'm not sure why putting it there worked, but I'll move it before inttypes.h, since that's where it's supposed to be.

Apparently the fixed-width types are also available in stdint.h if you define that macro. There's also a duplicate include of stdint.h in the .cpp file. I'm simplifying this a bit.

[Brendan Long]

Created attachment 222332 [details]
Patch

[Brendan Long]

Created attachment 222335 [details]
Patch

Hm, apparently the build bot's version of GCC doesn't like that either. How about using inttypes.h, with the macro where it should be?

[Brendan Long]

Oh, I see why I originally put in the .cpp file, but I think this version should work too. I was trying to get the macro defined before we imported *anything*, since there's some danger that any std*.h could put in inttypes.h or stdint.h. I think since the relevant code is in the .h file, it would be best if we can define it there too though.

[Brendan Long]

Or maybe it only works if it's defined in the .cpp file.. :\

[Brendan Long]

Maybe I should #define __STDC_FORMAT_MACROS in Platform.h? Or Compiler.h?

[Brendan Long]

Looks like someone else fixed this as a side effect of using this code in EFL.

*** This bug has been marked as a duplicate of bug 136089 ***