Closed Bug 1308897 Opened 8 years ago Closed 8 years ago

CodeCoverage.cpp is missing PRIu64 on several places

Categories

(Core :: JavaScript Engine, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: petr.sumbera, Unassigned)

References

Details

Attachments

(2 files)

Attached patch CodeCoverage.cpp.diff (deleted) — 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.
QA Whiteboard: [bugday-20161010]
Component: Untriaged → JavaScript: Standard Library
Product: Firefox → Core
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Attached patch Bug1308897.diff (deleted) — Splinter Review
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.
(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 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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: