Closed Bug 1232113 Opened 7 years ago Closed 7 years ago

Make the format specifiers in JS_snprintf() invocations more portable

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: wuwei, Assigned: wuwei)

References

Details

Attachments

(13 files, 4 obsolete files)

669 bytes, patch
jandem
: review+
Details | Diff | Splinter Review
941 bytes, patch
h4writer
: review+
Details | Diff | Splinter Review
972 bytes, patch
jonco
: review+
Details | Diff | Splinter Review
1.35 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
3.18 KB, patch
sunfish
: review+
Details | Diff | Splinter Review
834 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
1.17 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.65 KB, patch
nbp
: review+
Details | Diff | Splinter Review
962 bytes, patch
evilpie
: review+
Details | Diff | Splinter Review
788 bytes, patch
n.nethercote
: review+
Details | Diff | Splinter Review
769 bytes, patch
Details | Diff | Splinter Review
555 bytes, patch
Details | Diff | Splinter Review
1.11 KB, 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.
PRIu32, PRId64 are included in IntegerPrintfMacros.h (which includes inttypes.h), and PRIuSIZE is included in SizePrintfMacros.h.
Attachment #8697816 - Flags: review?(jdemooij)
the type of pid_ is size_t, so size_t(pid_) === pid_
Attachment #8697817 - Flags: review?(nicolas.b.pierron)
Attachment #8697821 - Flags: review?(jdemooij)
Attachment #8697822 - Flags: review?(n.nethercote)
Attachment #8697823 - Attachment description: Part 75: update specifier for uint32_t in LIR.cpp → Part 7: update specifier for uint32_t in LIR.cpp
Attachment #8697825 - Flags: review?(terrence)
Attachment #8697826 - Flags: review?(till)
Attachment #8697827 - Flags: review?(nicolas.b.pierron)
Attachment #8697828 - Flags: review?(n.nethercote)
Attachment #8697829 - Flags: review?(evilpies)
Attachment #8697830 - Flags: review?(shu)
Attachment #8697825 - Flags: review?(terrence) → review?(sphink)
Attachment #8697826 - Flags: review?(till) → review?(jwalden+bmo)
Attachment #8697822 - Flags: review?(n.nethercote) → review+
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-
Attachment #8697823 - Flags: review?(sunfish) → review+
Attachment #8697820 - Flags: review?(jcoppeard) → review+
(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
Attachment #8697828 - Attachment is obsolete: true
Attachment #8697951 - Flags: review?(n.nethercote)
Attachment #8697817 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8697827 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8697825 - Flags: review?(sphink) → review+
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 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+
(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 :)
Attachment #8697818 - Flags: review?(hv1989) → review+
Attachment #8697829 - Flags: review?(evilpies) → review+
Attachment #8697821 - Flags: review?(jdemooij) → review+
Attachment #8697816 - Flags: review?(jdemooij) → review+
Attachment #8697826 - Flags: review?(jwalden+bmo) → review+
rebased. r=nbp
Attachment #8697817 - Attachment is obsolete: true
updated. r=jandem
Attachment #8697821 - Attachment is obsolete: true
added missing ')'. r=shu
Attachment #8697830 - Attachment is obsolete: true
Keywords: checkin-needed
can you provide a try run for this changes, thanks!
Flags: needinfo?(lazyparser)
Keywords: checkin-needed
(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)
Keywords: checkin-needed
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)
(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)
Flags: needinfo?(lazyparser)
Flags: needinfo?(cbook)
See Also: → 1308964
You need to log in before you can comment on or make changes to this bug.