1232113 - Make the format specifiers in JS_snprintf() invocations more portable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

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.

Comment 1

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 1: Include headers for PRIuSIZE and PRIu32, etc.

PRIu32, PRId64 are included in IntegerPrintfMacros.h (which includes inttypes.h), and PRIuSIZE is included in SizePrintfMacros.h.

Comment 2

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 2: update format specifiers in CodeCoverage.cpp

the type of pid_ is size_t, so size_t(pid_) === pid_

Comment 3

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 3: update specifier for size_t in TraceLogging.cpp

Comment 4

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 4: update specifier for size_t in TestingFunctions.cpp

Comment 5

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 5: update specifier for uint32_t in jsopcode.cpp

Comment 6

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 6: update specifier for uint32_t in Tracer.cpp

Comment 7

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 7: update specifier for uint32_t in LIR.cpp

Comment 8

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 8: update format specifiers in jsgc.cpp

Comment 9

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 9: update format specifiers in jsobj.cpp

Comment 10

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 10: update format specifiers in shell/js.cpp

Comment 11

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 11: update format specifiers in vm/CharacterEncoding.cpp

Comment 12

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 12: update format specifiers in vm/JSONParser.cpp

Comment 13

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 13: update format specifiers in vm/SPSProfiler.cpp

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Wei Wu [:wuwei UTC+8]]

(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

Comment 16

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 11: update format specifiers in vm/CharacterEncoding.cpp

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Shu-yu Guo [:shu]]

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 )

Comment 19

[Wei Wu [:wuwei UTC+8]]

(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 :)

Comment 20

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 2: update format specifiers in CodeCoverage.cpp

rebased. r=nbp

Comment 21

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 5: update specifier for uint32_t in jsopcode.cpp

updated. r=jandem

Comment 22

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 13: update format specifiers in vm/SPSProfiler.cpp

added missing ')'. r=shu

Comment 23

[Carsten Book [:Tomcat]]

can you provide a try run for this changes, thanks!

Comment 24

[Wei Wu [:wuwei UTC+8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61f6f98b7a5a

Comment 25

[Wei Wu [:wuwei UTC+8]]

(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.

Comment 26

[Carsten Book [:Tomcat]]

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

Comment 27

[Wei Wu [:wuwei UTC+8]]

(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

Comment 28

[Pulsebot]

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

Comment 29

[Carsten Book [:Tomcat]]

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