Closed
Bug 1308897
Opened 8 years ago
Closed 8 years ago
CodeCoverage.cpp is missing PRIu64 on several places
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: petr.sumbera, Unassigned)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; SunOS i86pc; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160930185407
Steps to reproduce:
While running 'make check-jit-test' in js/src on sparc I have ran into an issue where the root cause is using "%d" instead of "%lld" in format string for uint64_t type. Please see attached patch with proposed fix.
Updated•8 years ago
|
QA Whiteboard: [bugday-20161010]
Component: Untriaged → JavaScript: Standard Library
Product: Firefox → Core
Comment 1•8 years ago
|
||
Thank you for your patch :D
LSprinter::printf internally calls JS_vsmprintf, that has its own format strings.
so, as you wrote in the comment, using %lld or %llu directly instead of PRI*64 would be the right solution.
https://dxr.mozilla.org/mozilla-central/rev/d72cf6ecebaf707c159f6697d9f5f6f6a6feb7e1/js/src/vm/Printer.cpp#576-602
> int
> LSprinter::printf(const char* fmt, ...)
> {
> ...
> int i = vprintf(fmt, va);
> ...
> }
>
> int
> LSprinter::vprintf(const char* fmt, va_list ap)
> {
> ...
> bp = JS_vsmprintf(fmt, ap); /* XXX vsaprintf */
> ...
> }
https://dxr.mozilla.org/mozilla-central/rev/d72cf6ecebaf707c159f6697d9f5f6f6a6feb7e1/js/src/jsprf.h#11-19
> /*
> ** API for PR printf like routines. Supports the following formats
> ** %d - decimal
> ** %u - unsigned decimal
> ** %x - unsigned hex
> ** %X - unsigned uppercase hex
> ** %o - unsigned octal
> ** %hd, %hu, %hx, %hX, %ho - 16-bit versions of above
> ** %ld, %lu, %lx, %lX, %lo - 32-bit versions of above
> ** %lld, %llu, %llx, %llX, %llo - 64 bit versions of above
Component: JavaScript: Standard Library → JavaScript Engine
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•8 years ago
|
||
Thanks for the bug report and submission.
To make sure we have the proper authorship (your name, email, and subject), you can follow the following rules [1], such that we can merge your patch once it is reviewed.
[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Updated•8 years ago
|
Reporter | ||
Comment 3•8 years ago
|
||
I have merged my patch to head (and addded requested formal info).
Though I'm not sure I fully follow that right fix is to use %llu isntead of PRI*64. There are other cases of PRI*64 in the file.
Comment 4•8 years ago
|
||
(In reply to Petr Sumbera from comment #3)
> Though I'm not sure I fully follow that right fix is to use %llu isntead of
> PRI*64. There are other cases of PRI*64 in the file.
sorry for confusion.
it was pre-existing issue that we're using PRI*64 where it might not be supported.
Filed bug 1308964 to track it.
Comment 5•8 years ago
|
||
Comment on attachment 8799460 [details] [diff] [review]
Bug1308897.diff
Review of attachment 8799460 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for your patch. :)
Considering that bug 1308964 is an existing issue, I do not think this patch changes much to the problem.
I will send this patch to our testing server, to make sure that it compiles well on all platforms [1], and then land this patch on mozilla-inbound tree, which is the integration branch of Firefox.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c87e3fdbdf869ac89a0140c1c41e6cd9fbedae8
Attachment #8799460 -
Flags: review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c01dab3fd041
CodeCoverage.cpp is missing PRIu64 on several places. r=nbp
Comment 7•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•