WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(1.80 KB, patch)
2013-12-31 15:14 PST
,
Brendan Long
no flags
Details
Formatted Diff
Diff
Patch
(2.41 KB, patch)
2014-01-02 16:20 PST
,
Brendan Long
no flags
Details
Formatted Diff
Diff
Patch
(2.41 KB, patch)
2014-01-03 07:21 PST
,
Brendan Long
no flags
Details
Formatted Diff
Diff
Patch
(2.37 KB, patch)
2014-01-27 10:15 PST
,
Brendan Long
no flags
Details
Formatted Diff
Diff
Patch
(2.39 KB, patch)
2014-01-27 10:24 PST
,
Brendan Long
msaboff
: review+
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Brendan Long
Comment 1
2013-12-31 14:59:08 PST
Created
attachment 220171
[details]
Patch
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
Created
attachment 220173
[details]
Patch
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
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
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
Created
attachment 220264
[details]
Patch
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
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
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
Created
attachment 220302
[details]
Patch
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
Created
attachment 222332
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug