RESOLVED DUPLICATE of bug 136089 126352
JSC disassembler warns about incorrect printf format on 64-bit Linux
https://bugs.webkit.org/show_bug.cgi?id=126352
Summary JSC disassembler warns about incorrect printf format on 64-bit Linux
Brendan Long
Reported 2013-12-31 14:56:37 PST
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));
Attachments
Patch (1.66 KB, patch)
2013-12-31 14:59 PST, Brendan Long
no flags
Patch (1.80 KB, patch)
2013-12-31 15:14 PST, Brendan Long
no flags
Patch (2.41 KB, patch)
2014-01-02 16:20 PST, Brendan Long
no flags
Patch (2.41 KB, patch)
2014-01-03 07:21 PST, Brendan Long
no flags
Patch (2.37 KB, patch)
2014-01-27 10:15 PST, Brendan Long
no flags
Patch (2.39 KB, patch)
2014-01-27 10:24 PST, Brendan Long
msaboff: review+
Brendan Long
Comment 1 2013-12-31 14:59:08 PST
Brendan Long
Comment 2 2013-12-31 15:00:20 PST
Does EWS not test Windows anymore? I really don't want to find a Windows computer to test this :\
Brendan Long
Comment 3 2013-12-31 15:14:43 PST
Brendan Long
Comment 4 2013-12-31 15:15:16 PST
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
Comment 5 2013-12-31 15:25:38 PST
Looks like I might need to setup a VM to figure out why EWS doesn't like this.
kov's GTK+ EWS bot
Comment 6 2013-12-31 15:37:39 PST
Zan Dobersek
Comment 7 2014-01-01 06:01:36 PST
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
Comment 8 2014-01-02 16:20:54 PST
Brendan Long
Comment 9 2014-01-02 16:21:43 PST
(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 10 2014-01-02 18:10:49 PST
Gyuyoung Kim
Comment 11 2014-01-03 02:28:53 PST
(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
Comment 12 2014-01-03 03:33:55 PST
(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
Comment 13 2014-01-03 03:42:40 PST
(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
Comment 14 2014-01-03 07:21:08 PST
(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
Comment 15 2014-01-03 07:21:41 PST
Brendan Long
Comment 16 2014-01-03 09:04:38 PST
Now it passes (same patch though).
Brendan Long
Comment 17 2014-01-17 12:20:17 PST
Can someone review this so we can commit it?
Philippe Normand
Comment 18 2014-01-26 20:30:22 PST
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
Comment 19 2014-01-27 09:58:39 PST
(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
Comment 20 2014-01-27 10:03:30 PST
(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
Comment 21 2014-01-27 10:11:36 PST
(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
Comment 22 2014-01-27 10:15:53 PST
Brendan Long
Comment 23 2014-01-27 10:24:25 PST
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
Comment 24 2014-01-27 10:27:00 PST
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
Comment 25 2014-01-27 10:36:21 PST
Or maybe it only works if it's defined in the .cpp file.. :\
Brendan Long
Comment 26 2014-01-27 11:48:47 PST
Maybe I should #define __STDC_FORMAT_MACROS in Platform.h? Or Compiler.h?
Brendan Long
Comment 27 2015-01-28 11:12:37 PST
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 ***
Note You need to log in before you can comment on or make changes to this bug.