Closed
Bug 1232113
Opened 9 years ago
Closed 9 years ago
Make the format specifiers in JS_snprintf() invocations more portable
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: wuwei, Assigned: wuwei)
References
Details
Attachments
(13 files, 4 obsolete files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sunfish
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Format specifiers in a few JS_snprintf() invocations are ad-hoc, involving unnecessarily type conversions and non-portable specifiers. Might be good to improve these.
Assignee | ||
Comment 1•9 years ago
|
||
PRIu32, PRId64 are included in IntegerPrintfMacros.h (which includes inttypes.h), and PRIuSIZE is included in SizePrintfMacros.h.
Attachment #8697816 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•9 years ago
|
||
the type of pid_ is size_t, so size_t(pid_) === pid_
Attachment #8697817 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8697818 -
Flags: review?(hv1989)
Assignee | ||
Updated•9 years ago
|
status-firefox45:
affected → ---
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8697820 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8697821 -
Flags: review?(jdemooij)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8697822 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8697823 -
Flags: review?(sunfish)
Assignee | ||
Updated•9 years ago
|
Attachment #8697823 -
Attachment description: Part 75: update specifier for uint32_t in LIR.cpp → Part 7: update specifier for uint32_t in LIR.cpp
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8697825 -
Flags: review?(terrence)
Assignee | ||
Updated•9 years ago
|
Attachment #8697826 -
Flags: review?(till)
Assignee | ||
Updated•9 years ago
|
Attachment #8697827 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Updated•9 years ago
|
Attachment #8697828 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•9 years ago
|
Attachment #8697829 -
Flags: review?(evilpies)
Assignee | ||
Updated•9 years ago
|
Attachment #8697830 -
Flags: review?(shu)
Assignee | ||
Updated•9 years ago
|
Attachment #8697825 -
Flags: review?(terrence) → review?(sphink)
Assignee | ||
Updated•9 years ago
|
Attachment #8697826 -
Flags: review?(till) → review?(jwalden+bmo)
Updated•9 years ago
|
Attachment #8697822 -
Flags: review?(n.nethercote) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8697828 [details] [diff] [review]
Part 11: update format specifiers in vm/CharacterEncoding.cpp
Review of attachment 8697828 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/vm/CharacterEncoding.cpp
@@ -202,5 @@
> static void
> ReportInvalidCharacter(JSContext* cx, uint32_t offset)
> {
> char buffer[10];
> - JS_snprintf(buffer, 10, "%d", offset);
As far as I know |uint32_t| is equivalent to |unsigned int| on all the platforms we care about and that's unlikely to change soon. So I would suggest just using |%u| here, unless you have strong arguments against this.
Attachment #8697828 -
Flags: review?(n.nethercote) → review-
Updated•9 years ago
|
Attachment #8697823 -
Flags: review?(sunfish) → review+
Updated•9 years ago
|
Attachment #8697820 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 15•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #14)
> Comment on attachment 8697828 [details] [diff] [review]
> Part 11: update format specifiers in vm/CharacterEncoding.cpp
>
> Review of attachment 8697828 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: js/src/vm/CharacterEncoding.cpp
> @@ -202,5 @@
> > static void
> > ReportInvalidCharacter(JSContext* cx, uint32_t offset)
> > {
> > char buffer[10];
> > - JS_snprintf(buffer, 10, "%d", offset);
>
> As far as I know |uint32_t| is equivalent to |unsigned int| on all the
> platforms we care about and that's unlikely to change soon. So I would
> suggest just using |%u| here, unless you have strong arguments against this.
According to the standard[1] "7.8.1 Macros for format specifiers", when we [f,s]printf
fixed width integral type, the corresponding specifiers are PRI[ouxd]N.
Indeed |unsigned int| is equivalent with |unit32_t| on ILP32, LP64 and LLP64 scheme,
while the size of |unsigned int| is 8 bytes in ILP64 scheme, 2 bytes in LP32 scheme.
ILP32 was used in win16; ILP64 was used in some SPARC64 and UNICOS systems. Neither of them is on tier 1.
So although I prefer PRIu32, replacing PRIu32 with 'u' is ok to me, and I am ok to update the patch.
[1]: http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8697828 -
Attachment is obsolete: true
Attachment #8697951 -
Flags: review?(n.nethercote)
Updated•9 years ago
|
Attachment #8697817 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Attachment #8697827 -
Flags: review?(nicolas.b.pierron) → review+
Updated•9 years ago
|
Attachment #8697825 -
Flags: review?(sphink) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8697951 [details] [diff] [review]
Part 11: update format specifiers in vm/CharacterEncoding.cpp
Review of attachment 8697951 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for the detailed explanation! To be truly consistent you'd likely have to change many %u specifiers to PRIu32 throughout the codebase, which wouldn't be worth the trouble. So this is a good compromise.
Attachment #8697951 -
Flags: review?(n.nethercote) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8697830 [details] [diff] [review]
Part 13: update format specifiers in vm/SPSProfiler.cpp
Review of attachment 8697830 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with right paren re-inserted.
::: js/src/vm/SPSProfiler.cpp
@@ +352,5 @@
> ? JS::CharsToNewUTF8CharsZ(nullptr, atom->latin1Range(nogc)).c_str()
> : JS::CharsToNewUTF8CharsZ(nullptr, atom->twoByteRange(nogc)).c_str());
> if (!atomStr)
> return nullptr;
> + ret = JS_snprintf(cstr, len + 1, "%s (%s:%" PRIu64, atomStr.get(), filename, lineno);
I think you missed the closing right paren )
Attachment #8697830 -
Flags: review?(shu) → review+
Assignee | ||
Comment 19•9 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #18)
> Comment on attachment 8697830 [details] [diff] [review]
> Part 13: update format specifiers in vm/SPSProfiler.cpp
>
> Review of attachment 8697830 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r=me with right paren re-inserted.
>
> ::: js/src/vm/SPSProfiler.cpp
> @@ +352,5 @@
> > ? JS::CharsToNewUTF8CharsZ(nullptr, atom->latin1Range(nogc)).c_str()
> > : JS::CharsToNewUTF8CharsZ(nullptr, atom->twoByteRange(nogc)).c_str());
> > if (!atomStr)
> > return nullptr;
> > + ret = JS_snprintf(cstr, len + 1, "%s (%s:%" PRIu64, atomStr.get(), filename, lineno);
>
> I think you missed the closing right paren )
ah, indeed. will fix.
Thank you :)
Updated•9 years ago
|
Attachment #8697818 -
Flags: review?(hv1989) → review+
Updated•9 years ago
|
Attachment #8697829 -
Flags: review?(evilpies) → review+
Updated•9 years ago
|
Attachment #8697821 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8697816 -
Flags: review?(jdemooij) → review+
Updated•9 years ago
|
Attachment #8697826 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 21•9 years ago
|
||
updated. r=jandem
Attachment #8697821 -
Attachment is obsolete: true
Assignee | ||
Comment 22•9 years ago
|
||
added missing ')'. r=shu
Attachment #8697830 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
can you provide a try run for this changes, thanks!
Flags: needinfo?(lazyparser)
Keywords: checkin-needed
Assignee | ||
Comment 24•9 years ago
|
||
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Wei Wu [:w :wuwei UTC+8] from comment #24)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=61f6f98b7a5a
Hi, this it the try run.
btw "OS X 10.7 debug" build succeed, actually.
Flags: needinfo?(lazyparser) → needinfo?(cbook)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 26•9 years ago
|
||
Hi, part7 failed to apply:
adding 1232113 to series file
renamed 1232113 -> bug1232113p7.patch
applying bug1232113p7.patch
patching file js/src/jit/LIR.cpp
Hunk #1 FAILED at 358
Hunk #2 FAILED at 385
Hunk #3 FAILED at 425
3 out of 3 hunks FAILED -- saving rejects to file js/src/jit/LIR.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1232113p7.patch
Flags: needinfo?(cbook) → needinfo?(lazyparser)
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #26)
> Hi, part7 failed to apply:
>
> adding 1232113 to series file
> renamed 1232113 -> bug1232113p7.patch
> applying bug1232113p7.patch
> patching file js/src/jit/LIR.cpp
> Hunk #1 FAILED at 358
> Hunk #2 FAILED at 385
> Hunk #3 FAILED at 425
> 3 out of 3 hunks FAILED -- saving rejects to file js/src/jit/LIR.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh bug1232113p7.patch
Hi Carsten,
the patch[1] for Bug 1225203 has landed in mozilla-inbound, which makes part7 unnecessary anymore.
So it would be ok to leave part7 not applied.
[1] https://hg.mozilla.org/integration/mozilla-inbound/diff/22bb9e57553b/js/src/jit/LIR.cpp
Flags: needinfo?(cbook)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(lazyparser)
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c816f80dd850
https://hg.mozilla.org/integration/mozilla-inbound/rev/df4f9202a8ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8b8f0a824e
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c6cb6e8625
https://hg.mozilla.org/integration/mozilla-inbound/rev/ada1474811b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec32289f301b
https://hg.mozilla.org/integration/mozilla-inbound/rev/242a17770fe0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99c2040f54c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2569a9980921
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4daad853e4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/99b0540a7821
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb0c3673bd3
Keywords: checkin-needed
Comment 29•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c816f80dd850
https://hg.mozilla.org/mozilla-central/rev/df4f9202a8ed
https://hg.mozilla.org/mozilla-central/rev/ab8b8f0a824e
https://hg.mozilla.org/mozilla-central/rev/34c6cb6e8625
https://hg.mozilla.org/mozilla-central/rev/ada1474811b7
https://hg.mozilla.org/mozilla-central/rev/ec32289f301b
https://hg.mozilla.org/mozilla-central/rev/242a17770fe0
https://hg.mozilla.org/mozilla-central/rev/c99c2040f54c
https://hg.mozilla.org/mozilla-central/rev/2569a9980921
https://hg.mozilla.org/mozilla-central/rev/b4daad853e4d
https://hg.mozilla.org/mozilla-central/rev/99b0540a7821
https://hg.mozilla.org/mozilla-central/rev/2eb0c3673bd3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(cbook)
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•