src: support UTF-8 in compiled-in JS source files #11129

bnoordhuis · 2017-02-02T22:39:09Z

Detect it when source files in lib/ are not ASCII. Decode them as UTF-8
and store them as UTF-16 in the binary so they can be used as external
string resources without non-ASCII characters getting mangled.

CI: https://ci.nodejs.org/job/node-test-pull-request/6176/

bnoordhuis · 2017-02-02T22:45:15Z

I rather feared this approach may be a bit too forward-looking for some compilers... I'll see if I can dumb it down.

aqrln · 2017-02-02T22:49:01Z

tools/js2c.py

+    # Treat non-ASCII as UTF-8 and convert it to UTF-16.
+    if any(ord(c) > 127 for c in lines):
+      ctype = 'uint16_t'
+      data = map(ord, lines.decode('utf-8').encode('utf-16be'))


Maybe 'utf-16be' if sys.byteorder == 'little' else 'utf-16le'?

The big-endian encoding is intentional, the line below stitches the individual octets together again into uint16s. (.encode() returns a byte string in case you're wondering.)

Ah, I see. Right.

jasnell · 2017-02-02T23:30:10Z

Just curious, have there been specific issues or is this purely preventative?

aqrln · 2017-02-02T23:35:44Z

@jasnell yes, reported in #10673

gibfahn · 2017-02-02T23:51:45Z

What's the reason for using UTF-16 rather than UTF-8 (out of interest) to store the files in the binary?

jasnell · 2017-02-02T23:55:26Z

I would have to assume it is because the external string API does not directly support UTF8. That is, if you look at ExternalStringResource, it assumes a utf16_t* buffer while ExternalOneByteStringResource assumes const char*. If the strings were stored as UTF8 we would incur an additional cost at startup that is not necessary.

aqrln · 2017-02-02T23:55:39Z

@gibfahn so that they can be used as external strings. UTF-8 strings must be copied to V8's managed memory. See #5458 by @bnoordhuis.

addaleax · 2017-02-03T07:17:41Z

src/node_javascript.cc

 using v8::Object;
 using v8::String;

+template <typename T, size_t N, T P>


Tiny suggestion: Could we be a bit more explicit and rename P to Ptr or Pointer? 😄

aqrln · 2017-02-03T17:59:14Z

@bnoordhuis ping. Did you have time to dumb it down? 😉 This patch seems to work on GCC 4.8+.

Diff

diff --git a/src/node_javascript.cc b/src/node_javascript.cc
index 8c665381db..07a0daaa83 100644
--- a/src/node_javascript.cc
+++ b/src/node_javascript.cc
@@ -16,16 +16,16 @@ using v8::String;
 template <typename T, size_t N, T P>
 struct ExternalStringResource;
 
-template <size_t N, const char (&P)[N]>
-struct ExternalStringResource<const char[N], N, P>
+template <size_t N, const char* P>
+struct ExternalStringResource<const char*, N, P>
     : public String::ExternalOneByteStringResource {
   const char* data() const override { return P; }
   size_t length() const override { return N; }
   void Dispose() override { /* Default calls `delete this`. */ }
 };
 
-template <size_t N, const uint16_t (&P)[N]>
-struct ExternalStringResource<const uint16_t[N], N, P>
+template <size_t N, const uint16_t* P>
+struct ExternalStringResource<const uint16_t*, N, P>
     : public String::ExternalStringResource {
   const uint16_t* data() const override { return P; }
   size_t length() const override { return N; }
@@ -34,7 +34,7 @@ struct ExternalStringResource<const uint16_t[N], N, P>
 
 // id##_data is defined in node_natives.h.
 #define V(id) \
-  static ExternalStringResource<decltype(id##_data),                          \
+  static ExternalStringResource<decltype(&id##_data[0]),                      \
                                 arraysize(id##_data),                         \
                                 id##_data> id##_external_data;
 NODE_NATIVES_MAP(V)
diff --git a/tools/js2c.py b/tools/js2c.py
index e6c56cf075..8104e50e5a 100755
--- a/tools/js2c.py
+++ b/tools/js2c.py
@@ -189,7 +189,7 @@ NODE_NATIVES_MAP = """\
 SOURCES = """\
 static const uint8_t {escaped_id}_name[] = {{
 {name}}};
-static const {ctype} {escaped_id}_data[] = {{
+static constexpr const {ctype} {escaped_id}_data[] = {{
 {data}}};
 """

bnoordhuis · 2017-02-06T19:52:48Z

@aqrln Thanks. I tried a couple of different approaches but I ended up settling for something that's pretty similar to your patch. :-)

I added a second commit that replaces a few U+2019 quotes with ASCII quotes so that their files can be stored as one-byte strings. Can I get a quick LGTM or two?

EDIT: The CI scoreboard is currently linking to a previous CI build. The correct one is https://ci.nodejs.org/job/node-test-commit/7713/.

richardlau · 2017-02-06T20:05:04Z

I added a second commit that replaces a few U+2019 quotes with ASCII quotes so that their files can be stored as one-byte strings.

Probably one for another PR, but this sounds like a good candidate for a lint rule.

bnoordhuis · 2017-02-06T20:31:47Z

Interesting VS build failure...

an expression involving objects with internal linkage cannot be used as a non-type argument

I suppose the compiler is technically correct, and wasn't that the best kind of correct?

EDIT: New attempt: https://ci.nodejs.org/job/node-test-pull-request/6243/

bnoordhuis · 2017-02-11T21:01:08Z

Different take: moved all the logic to js2c.py.

CI: https://ci.nodejs.org/job/node-test-pull-request/6357/

EDIT: Sigh...

node_javascript.cc(55276): error C2466: cannot allocate an array of constant size 0

bnoordhuis · 2017-02-12T10:15:07Z

CI is finally green except for that silly issue where the ARM buildbots don't report their status properly.

@addaleax The PR changed quite a bit, perhaps you want to take another look?

bnoordhuis · 2017-02-12T10:18:40Z

node.gyp

You'd expect INTERMEDIATE_DIR to work here but it doesn't: js2c generates the file but the build doesn't pick it up (and I updated the 'sources' list in case you're wondering.) I decided to leave well enough alone for now.

Detect it when source files in lib/ are not ASCII. Decode them as UTF-8 and store them as UTF-16 in the binary so they can be used as external string resources without non-ASCII characters getting mangled. Fixes: nodejs#10673 PR-URL: nodejs#11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

The previous commit stores baked-in files with non-ASCII characters as UTF-16. Replace the \u2019 with a regular quote character so that the files they're in can be stored as one-byte strings. The UTF-16 functionality is still tested by the Unicode diagram in lib/timers.js. PR-URL: nodejs#11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Detect it when source files in lib/ are not ASCII. Decode them as UTF-8 and store them as UTF-16 in the binary so they can be used as external string resources without non-ASCII characters getting mangled. Fixes: nodejs#10673 PR-URL: nodejs#11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

The previous commit stores baked-in files with non-ASCII characters as UTF-16. Replace the \u2019 with a regular quote character so that the files they're in can be stored as one-byte strings. The UTF-16 functionality is still tested by the Unicode diagram in lib/timers.js. PR-URL: nodejs#11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

In order to allow using Unicode characters inside comments of built-in JavaScript libraries without forcing them to be stored as UTF-16 data in Node's binary, update the tooling to strip comments during build process. All line breaks are preserved so that line numbers in stack traces aren't broken. Refs: nodejs#11129 Refs: nodejs#11371 (comment)

This test ensures that UTF-8 characters can be used in core JavaScript modules built into Node's binary. Refs: nodejs#11129

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](#11094) * add node-inspect 1.10.2 (Jan Krems) [#10187](#10187) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](#11149) * fs: allow WHATWG URL and file: URLs as paths (James M Snell) [#10739](#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](#10857) PR-URL: #11185

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [nodejs#11029](nodejs#11029) * upgrade libuv to 1.11.0 (cjihrig) [nodejs#11094](nodejs#11094) * add node-inspect 1.10.4 (Jan Krems) [nodejs#10187](nodejs#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [nodejs#10980](nodejs#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [nodejs#10187](nodejs#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [nodejs#9469](nodejs#9469) * inspector: add --inspect-brk (Josh Gavant) [nodejs#11149](nodejs#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [nodejs#10739](nodejs#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [nodejs#11129](nodejs#11129) * url: extend url.format to support WHATWG URL (James M Snell) [nodejs#10857](nodejs#10857) PR-URL: nodejs#11185

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094) * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857) PR-URL: nodejs/node#11185 Signed-off-by: Ilkka Myller <[email protected]>

jasnell · 2017-03-07T18:01:54Z

Needs a backport PR to land in v4.
Only one of the two commits was cherry-picked into v6 (the second one wasn't needed)

Detect it when source files in lib/ are not ASCII. Decode them as UTF-8 and store them as UTF-16 in the binary so they can be used as external string resources without non-ASCII characters getting mangled. Fixes: #10673 PR-URL: #11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test ensures that UTF-8 characters can be used in core JavaScript modules built into Node's binary. PR-URL: #11423 Ref: #11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]>

This test ensures that UTF-8 characters can be used in core JavaScript modules built into Node's binary. PR-URL: nodejs#11423 Ref: nodejs#11129 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Feb 2, 2017

bnoordhuis mentioned this pull request Feb 2, 2017

utf8 problem in _third_party_main.js #10673

Closed

aqrln reviewed Feb 2, 2017

View reviewed changes

addaleax approved these changes Feb 3, 2017

View reviewed changes

bnoordhuis force-pushed the fix10673 branch from d9490e3 to fe24f7b Compare February 6, 2017 19:38

gibfahn mentioned this pull request Feb 7, 2017

Linter rule: use ASCII quotes not Unicode ones #11209

Closed

bnoordhuis force-pushed the fix10673 branch from 5151ccd to cb92703 Compare February 11, 2017 20:58

bnoordhuis force-pushed the fix10673 branch from cb92703 to 3076d8b Compare February 11, 2017 21:46

bnoordhuis commented Feb 12, 2017

View reviewed changes

addaleax approved these changes Feb 12, 2017

View reviewed changes

jasnell approved these changes Feb 12, 2017

View reviewed changes

bnoordhuis added 2 commits February 13, 2017 19:16

bnoordhuis force-pushed the fix10673 branch from 3076d8b to fd8587e Compare February 13, 2017 18:24

bnoordhuis closed this Feb 13, 2017

bnoordhuis deleted the fix10673 branch February 13, 2017 18:24

bnoordhuis merged commit fd8587e into nodejs:master Feb 13, 2017

thefourtheye mentioned this pull request Feb 14, 2017

tools: add ASCII only lint rule in lib/ #11371

Closed

3 tasks

italoacasas mentioned this pull request Feb 14, 2017

Proposal - 7.6.0 (Current) #11185

Merged

aqrln mentioned this pull request Feb 16, 2017

tools: make js2c.py strip comments from sources #11417

Closed

2 tasks

aqrln added a commit to aqrln/node that referenced this pull request Feb 16, 2017

test: add Unicode characters regression test

6fb4942

This test ensures that UTF-8 characters can be used in core JavaScript modules built into Node's binary. Refs: nodejs#11129

aqrln mentioned this pull request Feb 16, 2017

test: add Unicode characters regression test #11423

Closed

3 tasks

jasnell added the dont-land-on-v4.x label Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

joyeecheung mentioned this pull request Mar 28, 2017

Suspect memory leak in v6.10.1 #12033

Closed

MylesBorins added the backported-to-v6.x label Apr 17, 2017

SirR4T mentioned this pull request Jan 10, 2018

tools: non-Ascii linter for /lib only #18043

Closed

2 tasks

Uh oh!

src: support UTF-8 in compiled-in JS source files #11129

src: support UTF-8 in compiled-in JS source files #11129

Uh oh!

Conversation

bnoordhuis commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Feb 2, 2017

Uh oh!

aqrln Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

aqrln commented Feb 2, 2017

Uh oh!

gibfahn commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Feb 2, 2017

Uh oh!

aqrln commented Feb 2, 2017

Uh oh!

addaleax Feb 3, 2017

Choose a reason for hiding this comment

Uh oh!

aqrln commented Feb 3, 2017

Uh oh!

bnoordhuis commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Feb 6, 2017

Uh oh!

bnoordhuis commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Feb 12, 2017

Uh oh!

bnoordhuis Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

Uh oh!

bnoordhuis commented Feb 2, 2017 •

edited

Loading

gibfahn commented Feb 2, 2017 •

edited

Loading

bnoordhuis commented Feb 6, 2017 •

edited

Loading

bnoordhuis commented Feb 6, 2017 •

edited

Loading

bnoordhuis commented Feb 11, 2017 •

edited

Loading