V8 icu patch 4.x #15562

MylesBorins · 2017-09-22T18:51:16Z

MylesBorins · 2017-09-22T18:53:57Z

ci: https://ci.nodejs.org/job/node-test-pull-request/10205/
v8-ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/923/

it is worth mentioning some of the tests are failing in CI due to timeouts... I believe this is due to snapshots not having been re-enabled on 4.x

ci 4.8.4 for comparison: https://ci.nodejs.org/job/node-test-commit/12533/

MylesBorins · 2017-09-22T19:52:21Z

@nodejs/build are the errors we are seeing in V8 CI due to snapshots being turned off?

Qantas94Heavy

Sorry for not making this clear, but all of the instances of %_Call need to be changed to %_CallFunction, as V8 bug 4413 was not merged into V8 4.5.

Qantas94Heavy · 2017-09-23T00:19:37Z

deps/v8/src/i18n.js

+ * 'of', 'au' and 'es' are special-cased and lowercased.
+ */
+function toTitleCaseTimezoneLocation(location) {
+  var match = %_Call(StringMatch, location, GetTimezoneNameLocationPartRE());


var match = %_CallFunction(location, GetTimezoneNameLocationPartRE(), StringMatch);

Qantas94Heavy · 2017-09-23T00:22:16Z

deps/v8/src/i18n.js

+    // The first character is a separator, '_' or '-'.
+    // None of IANA zone names has both '_' and '-'.
+    var separator = %_Call(StringSubstring, match[2], 0, 1);
+    var parts = %_Call(StringSplit, match[2], separator);


var separator = %_CallFunction(match[2], 0, 1, StringSubstring); var parts = %_CallFunction(match[2], separator, StringSplit);

Qantas94Heavy · 2017-09-23T00:23:14Z

deps/v8/src/i18n.js

-    result = result + '_' + toTitleCaseWord(match[i]);
-    i++;
+  if (!IS_UNDEFINED(match[3]) && 3 < match.length) {
+    var locations = %_Call(StringSplit, match[3], '/');


var locations = %_CallFunction(match[3], '/', StringSplit);

jasnell

rubber stamp lgtm

MylesBorins · 2017-09-26T03:54:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10270/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/927/

/cc @nodejs/lts @nodejs/v8 @bnoordhuis

bnoordhuis · 2017-09-26T14:18:57Z

deps/v8/src/i18n.js

+ * 'of', 'au' and 'es' are special-cased and lowercased.
+ */
+function toTitleCaseTimezoneLocation(location) {
+  var match = %_CallFunction(StringMatch, location, GetTimezoneNameLocationPartRE());


%_CallFunction() takes the receiver as its first argument and the function as its last argument, everything in between are function arguments. I.e., this should read:

var match = %_CallFunction(location, GetTimezoneNameLocationPartRE(), StringMatch);

(Which is > 80 columns, by the way.)

bnoordhuis · 2017-09-26T14:19:38Z

deps/v8/src/i18n.js

+  if (!IS_UNDEFINED(match[2]) && 2 < match.length) {
+    // The first character is a separator, '_' or '-'.
+    // None of IANA zone names has both '_' and '-'.
+    var separator = %_CallFunction(StringSubstring, match[2], 0, 1);


Likewise, this should read:

var separator = %_CallFunction(match[2], 0, 1, StringSubstring);

I'll stop pointing it out from here on.

MylesBorins · 2017-09-27T17:41:08Z

good catch @bnoordhuis. fixed

ci: https://ci.nodejs.org/job/node-test-pull-request/10288/
V8-ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/930/

bnoordhuis

LGTM but apparently the v4.x branch doesn't build on the V8 buildbots... the only reason the rhel72s-s390x buildbot is green is because it skips the build entirely.

bnoordhuis · 2017-09-27T19:09:49Z

deps/v8/src/i18n.js

+ * 'of', 'au' and 'es' are special-cased and lowercased.
+ */
+function toTitleCaseTimezoneLocation(location) {
+  var match = %_CallFunction(location, GetTimezoneNameLocationPartRE(), StringMatch);


Just noting that this is over 80 columns. I can live with that, though.

MylesBorins · 2017-09-28T04:53:50Z

Here's a run on v4.8.4 to confirm that ci is just not working

https://ci.nodejs.org/job/node-test-commit-v8-linux/933/

If that is the case I will attempt to run the tests manually

edit: confirmed 4.x V8 ci is broken.

MylesBorins · 2017-09-29T20:56:23Z

I cannot get make test-v8 to work on any machine I've been testing it on... there are things breaking during compilation

In file included from ../src/lithium-allocator.cc:8:
In file included from .././src/lithium-inl.h:8:
.././src/lithium.h:321:45: error: instantiation of variable 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::REGISTER, 16>::cache' required here, but no definition is available
      [-Werror,-Wundefined-var-template]
    if (index < kNumCachedOperands) return &cache[index];
                                            ^
../src/lithium-allocator.cc:203:25: note: in instantiation of member function 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::REGISTER, 16>::Create' requested here
        op = LRegister::Create(assigned_register(), zone);
                        ^
.././src/lithium.h:334:27: note: forward declaration of template entity is here
  static LSubKindOperand* cache;
                          ^
.././src/lithium.h:321:45: note: add an explicit instantiation declaration to suppress this warning if 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::REGISTER, 16>::cache' is explicitly
      instantiated in another translation unit
    if (index < kNumCachedOperands) return &cache[index];
                                            ^
1 error generated.

This appears to be broken in v4.8.4 as well, testing v4.8.3 rn

edit: confirm that this fails in v4.8.3 as well

/cc @nodejs/v8

hashseed · 2017-09-30T07:37:01Z

Maybe because that version of V8 requires some old version of GCC?

bnoordhuis · 2017-10-02T19:56:16Z

@MylesBorins Untested, but I speculate the google-clang (or whatever you call it) that make-v8.sh checks out is no longer compatible with that version of V8. Does this patch help?

diff --git a/Makefile b/Makefile
index d917056b11..93356930af 100644
--- a/Makefile
+++ b/Makefile
@@ -191,7 +191,8 @@ endif
 
 v8:
 	tools/make-v8.sh
-	$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)
+	$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) \
+		$(V8_BUILD_OPTIONS) GYPFLAGS="-Dclang=0"
 
 ifeq ($(NODE_TARGET_TYPE),static_library)
 test: all

(Note: diff with tabs.)

MylesBorins · 2017-10-02T20:13:25Z

Getting a failure with that patch as well

../src/bignum.cc: In member function ‘void v8::internal::Bignum::AssignDecimalString(v8::internal::Vector<const char>)’:
../src/bignum.cc:82:6: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow]
 void Bignum::AssignDecimalString(Vector<const char> value) {
      ^
At global scope:
cc1plus: error: unrecognized command line option ‘-Wno-format-pedantic’ [-Werror]
cc1plus: all warnings being treated as errors
tools/gyp/v8_base.target.x64.release.mk:490: recipe for target '/home/mborins/node/deps/v8/out/x64.release/obj.target/v8_base/src/bignum.o' failed
make[2]: *** [/home/mborins/node/deps/v8/out/x64.release/obj.target/v8_base/src/bignum.o] Error 1
make[2]: Leaving directory '/home/mborins/node/deps/v8/out'
Makefile:302: recipe for target 'x64.release' failed
make[1]: *** [x64.release] Error 2
make[1]: Leaving directory '/home/mborins/node/deps/v8'
Makefile:114: recipe for target 'v8' failed
make: *** [v8] Error 2

edit: found a patch to V8 that may fix that

v8/v8@d7f813b

bnoordhuis · 2017-10-02T20:31:23Z

Ah, I think that is a bug in deps/v8/build/standalone.gypi - it adds that -Wno-format-pedantic flag even when clang==0. What happens when you delete it from that file?

MylesBorins · 2017-10-02T20:40:32Z

I've got another branch I've put together that attempts to fix this, testing before force pushing over this PR

https://github.com/MylesBorins/node/tree/V8-icu-patch-4.x-too

I saw failures on both linux + osx when manually testing, will edit this comment with the errors once I have a chance to re-run the tests

osx failures appear to be a git problem... ignoreing this one for now to focus on the linux issue

EAD is now at 87b733840e deps: backport 4e18190 from V8 upstream
Auto-merging src/string_bytes.cc
CONFLICT (content): Merge conflict in src/string_bytes.cc
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C deps/v8 x64.release \
		 GYPFLAGS="-Dclang=0"
PYTHONPATH="/Users/mborins/code/node/v4.x/deps/v8/tools/generate_shim_headers:/Users/mborins/code/node/v4.x/deps/v8/build::/Users/mborins/code/node/v4.x/deps/v8/build/gyp/pylib:" \
	GYP_GENERATORS=make \
	build/gyp/gyp --generator-output="out" build/all.gyp \
	              -Ibuild/standalone.gypi --depth=. \
	              -Dv8_target_arch=x64 \
	               -Dtarget_arch=x64 \
	               \
	              -S.x64.release -Dclang=0
  CXX(target) /Users/mborins/code/node/v4.x/deps/v8/out/x64.release/obj.target/v8_base/src/accessors.o
In file included from ../src/accessors.cc:5:
In file included from .././src/v8.h:29:
.././include/v8.h:353:33: error: use of undeclared identifier 'nullptr'
  V8_INLINE MaybeLocal() : val_(nullptr) {}
                                ^
.././include/v8.h:360:51: error: use of undeclared identifier 'nullptr'
  V8_INLINE bool IsEmpty() const { return val_ == nullptr; }
                                                  ^
.././include/v8.h:364:29: error: use of undeclared identifier 'nullptr'
    out->val_ = IsEmpty() ? nullptr : this->val_;
                            ^
.././include/v8.h:430:50: error: use of undeclared identifier 'nullptr'
  bool IsFirstPass() const { return callback_ != nullptr; }
                                                 ^
.././include/v8.h:469:29: error: alias declarations are a C++11 extension [-Werror,-Wc++11-extensions]
using PhantomCallbackData = WeakCallbackInfo<T>;
                            ^
.././include/v8.h:472:6: error: scoped enumerations are a C++11 extension [-Werror,-Wc++11-extensions]
enum class WeakCallbackType { kParameter, kInternalFields };
     ^
.././include/v8.h:637:43: error: deleted function definitions are a C++11 extension [-Werror,-Wc++11-extensions]
  PersistentBase(PersistentBase& other) = delete;  // NOLINT
                                          ^
.././include/v8.h:638:37: error: deleted function definitions are a C++11 extension [-Werror,-Wc++11-extensions]
  void operator=(PersistentBase&) = delete;
                                    ^
.././include/v8.h:814:26: error: rvalue references are a C++11 extension [-Werror,-Wc++11-extensions]
  V8_INLINE Global(Global&& other) : PersistentBase<T>(other.val_) {
                         ^
.././include/v8.h:822:40: error: rvalue references are a C++11 extension [-Werror,-Wc++11-extensions]
  V8_INLINE Global& operator=(Global<S>&& rhs) {
                                       ^
.././include/v8.h:844:21: error: deleted function definitions are a C++11 extension [-Werror,-Wc++11-extensions]
  Global(Global&) = delete;
                    ^
.././include/v8.h:845:29: error: deleted function definitions are a C++11 extension [-Werror,-Wc++11-extensions]
  void operator=(Global&) = delete;
                            ^
.././include/v8.h:790:42: error: use of undeclared identifier 'nullptr'
  V8_INLINE Global() : PersistentBase<T>(nullptr) {}
                                         ^
.././include/v8.h:815:18: error: use of undeclared identifier 'nullptr'
    other.val_ = nullptr;
                 ^
.././include/v8.h:827:18: error: use of undeclared identifier 'nullptr'
      rhs.val_ = nullptr;
                 ^
.././include/v8.h:834:44: error: rvalue references are a C++11 extension [-Werror,-Wc++11-extensions]
  Global Pass() { return static_cast<Global&&>(*this); }
                                           ^
.././include/v8.h:852:26: error: alias declarations are a C++11 extension [-Werror,-Wc++11-extensions]
using UniquePersistent = Global<T>;
                         ^
.././include/v8.h:1092:52: error: use of undeclared identifier 'nullptr'
                            ScriptOrigin* origin = nullptr));
                                                   ^
.././include/v8.h:1095:30: error: use of undeclared identifier 'nullptr'
      ScriptOrigin* origin = nullptr);
                             ^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
make[2]: *** [/Users/mborins/code/node/v4.x/deps/v8/out/x64.release/obj.target/v8_base/src/accessors.o] Error 1
make[1]: *** [x64.release] Error 2
make: *** [v8] Error 2

MylesBorins · 2017-10-02T20:51:43Z

Linux error on new branch

../src/bignum.cc: In member function ‘void v8::internal::Bignum::AssignDecimalString(v8::internal::Vector<const char>)’:
../src/bignum.cc:82:6: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow]
 void Bignum::AssignDecimalString(Vector<const char> value) {
      ^
cc1plus: all warnings being treated as errors
tools/gyp/v8_base.target.x64.release.mk:487: recipe for target '/home/mborins/node/deps/v8/out/x64.release/obj.target/v8_base/src/bignum.o' failed
make[2]: *** [/home/mborins/node/deps/v8/out/x64.release/obj.target/v8_base/src/bignum.o] Error 1
make[2]: Leaving directory '/home/mborins/node/deps/v8/out'
Makefile:302: recipe for target 'x64.release' failed
make[1]: *** [x64.release] Error 2
make[1]: Leaving directory '/home/mborins/node/deps/v8'
Makefile:114: recipe for target 'v8' failed
make: *** [v8] Error 2

edit: looks like this patch will fix it v8/v8@e28183b

MylesBorins · 2017-10-11T08:15:16Z

It works 🎉

I'm not sure how I feel about introducing export to the makefile... can someone sign off on this

ci: https://ci.nodejs.org/job/node-test-pull-request/10610/
ci-v8: https://ci.nodejs.org/job/node-test-commit-v8-linux/986/

edit
4.8.4 CI to compare failures on arm: https://ci.nodejs.org/job/node-test-commit/13029/

edit 2:
one more ci: https://ci.nodejs.org/job/node-test-pull-request/10615/
one more 4.8.4: https://ci.nodejs.org/job/node-test-commit/13036/

bnoordhuis · 2017-10-11T17:48:43Z

Makefile

 v8:
 	tools/make-v8.sh
-	$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)
+	$(MAKE) -C deps/v8 $(V8_ARCH).$(BUILDTYPE_LOWER) \
+		$(V8_BUILD_OPTIONS)


Can you undo this change?

Original commit message: Allow ICU to normalize time zones There's at least one case of a time zone alias: Asia/Kathmandu aliases Asia/Katmandu. ICU seems to normalize to the (deprecated) latter choice. V8 internationalization choked on this change; this patch interprets ICU's output more precisely and allows it. BUG=chromium:487322 R=jungshik,adamk LOG=Y Review URL: https://codereview.chromium.org/1509273007 Cr-Commit-Position: refs/heads/master@{nodejs#32769} PR-URL: nodejs#15562

Original commit message: Timezone name check fix 1. Location names with more than one underscores (e.g. Ho_Chi_Minh) didn't work because of the way capturing works with repeated patterns in RE. It's now supported by changing the RE to capture the whole string and splitting on '_' in the next step. 2. Adds support for location names with a hyphen 3. Adds support for timezone ids with three parts (e.g. American/Argentina/Buenos_Aires) 4. Adds special handling of 'au', 'es' and 'of' in zone ids. They need to be kept in lowercase. (see the full list at https://en.wikipedia.org/wiki/List_of_tz_database_time_zones ) 5. Adds regression tests for all the above and make the existing tests more robust against future ICU changes. ICU canonicalizes zone names to deprecated names, but it may change. ( http://bugs.icu-project.org/trac/ticket/12044 ) BUG=364374 LOG=Y Review URL: https://codereview.chromium.org/1529363005 Cr-Commit-Position: refs/heads/master@{nodejs#33097} PR-URL: nodejs#15562

MylesBorins · 2017-10-11T18:30:53Z

Makefile

@@ -110,6 +110,7 @@ check: test
 cctest: all
 	@out/$(BUILDTYPE)/$@

+v8: export GYPFLAGS += -Dclang=0


This is something I want explicit sign off on... is it ok to use export here?

Yep, seems fine.

srl295 · 2017-10-12T17:43:27Z

I'm not sure what the clang=0 has to do with things… but if it works, shrug

I didn't get to compare what other build files had changed in the intervening time. (busy trying to get ICU 60 out the door, etc etc).

the ICU included by V8 is le only

Yes, that's because of choices v8 has made with how it builds ICU. However, when Node builds v8, v8's ICU is out of the picture.

srl295

No objection…

The v8 and test-hash-seed targets cannot be run in parallel because they need different copies of the deps/v8 directory. Ref: #14004 (comment) Backport-PR-URL: #15562 PR-URL: #14219 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Original commit message: Fix compilation with GCC 5.2 Fixes: ../../test/cctest/compiler/test-js-typed-lowering.cc:224:14: error: ‘kJSTypes’ defined but not used [-Werror=unused-variable] static Type* kJSTypes[] = {Type::Undefined(), Type::Null(), Type::Boolean(), ../../src/bignum.cc: In member function ‘void v8::internal::Bignum::AssignDecimalString(Vector<const char>)’: ../../src/bignum.cc:80:6: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow] ../../src/compiler/ia32/code-generator-ia32.cc:1366:3: required from here ../../src/base/logging.h:123:26: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] DEFINE_CHECK_OP_IMPL(EQ, ==) BUG= Review URL: https://codereview.chromium.org/1371823002 Cr-Commit-Position: refs/heads/master@{#31095} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: GYP: Don't pass -Wno-format-pedantic to GCC. This flag is not understood correctly by GCC and breaks the GCC ARM and MIPS optdebug builds. Patch from Brendan Kirby <brendan.kirby@imgtec.com> BUG= Review URL: https://codereview.chromium.org/1369273003 Cr-Commit-Position: refs/heads/master@{#31013} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Allow ICU to normalize time zones There's at least one case of a time zone alias: Asia/Kathmandu aliases Asia/Katmandu. ICU seems to normalize to the (deprecated) latter choice. V8 internationalization choked on this change; this patch interprets ICU's output more precisely and allows it. BUG=chromium:487322 R=jungshik,adamk LOG=Y Review URL: https://codereview.chromium.org/1509273007 Cr-Commit-Position: refs/heads/master@{#32769} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Timezone name check fix 1. Location names with more than one underscores (e.g. Ho_Chi_Minh) didn't work because of the way capturing works with repeated patterns in RE. It's now supported by changing the RE to capture the whole string and splitting on '_' in the next step. 2. Adds support for location names with a hyphen 3. Adds support for timezone ids with three parts (e.g. American/Argentina/Buenos_Aires) 4. Adds special handling of 'au', 'es' and 'of' in zone ids. They need to be kept in lowercase. (see the full list at https://en.wikipedia.org/wiki/List_of_tz_database_time_zones ) 5. Adds regression tests for all the above and make the existing tests more robust against future ICU changes. ICU canonicalizes zone names to deprecated names, but it may change. ( http://bugs.icu-project.org/trac/ticket/12044 ) BUG=364374 LOG=Y Review URL: https://codereview.chromium.org/1529363005 Cr-Commit-Position: refs/heads/master@{#33097} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

MylesBorins · 2017-10-18T19:19:56Z

landed in fc6145f...4e44b64

The v8 and test-hash-seed targets cannot be run in parallel because they need different copies of the deps/v8 directory. Ref: #14004 (comment) Backport-PR-URL: #15562 PR-URL: #14219 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Original commit message: Fix compilation with GCC 5.2 Fixes: ../../test/cctest/compiler/test-js-typed-lowering.cc:224:14: error: ‘kJSTypes’ defined but not used [-Werror=unused-variable] static Type* kJSTypes[] = {Type::Undefined(), Type::Null(), Type::Boolean(), ../../src/bignum.cc: In member function ‘void v8::internal::Bignum::AssignDecimalString(Vector<const char>)’: ../../src/bignum.cc:80:6: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow] ../../src/compiler/ia32/code-generator-ia32.cc:1366:3: required from here ../../src/base/logging.h:123:26: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] DEFINE_CHECK_OP_IMPL(EQ, ==) BUG= Review URL: https://codereview.chromium.org/1371823002 Cr-Commit-Position: refs/heads/master@{#31095} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: GYP: Don't pass -Wno-format-pedantic to GCC. This flag is not understood correctly by GCC and breaks the GCC ARM and MIPS optdebug builds. Patch from Brendan Kirby <brendan.kirby@imgtec.com> BUG= Review URL: https://codereview.chromium.org/1369273003 Cr-Commit-Position: refs/heads/master@{#31013} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Allow ICU to normalize time zones There's at least one case of a time zone alias: Asia/Kathmandu aliases Asia/Katmandu. ICU seems to normalize to the (deprecated) latter choice. V8 internationalization choked on this change; this patch interprets ICU's output more precisely and allows it. BUG=chromium:487322 R=jungshik,adamk LOG=Y Review URL: https://codereview.chromium.org/1509273007 Cr-Commit-Position: refs/heads/master@{#32769} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Timezone name check fix 1. Location names with more than one underscores (e.g. Ho_Chi_Minh) didn't work because of the way capturing works with repeated patterns in RE. It's now supported by changing the RE to capture the whole string and splitting on '_' in the next step. 2. Adds support for location names with a hyphen 3. Adds support for timezone ids with three parts (e.g. American/Argentina/Buenos_Aires) 4. Adds special handling of 'au', 'es' and 'of' in zone ids. They need to be kept in lowercase. (see the full list at https://en.wikipedia.org/wiki/List_of_tz_database_time_zones ) 5. Adds regression tests for all the above and make the existing tests more robust against future ICU changes. ICU canonicalizes zone names to deprecated names, but it may change. ( http://bugs.icu-project.org/trac/ticket/12044 ) BUG=364374 LOG=Y Review URL: https://codereview.chromium.org/1529363005 Cr-Commit-Position: refs/heads/master@{#33097} PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Fix compilation with GCC 5.2 Fixes: ../../test/cctest/compiler/test-js-typed-lowering.cc:224:14: error: ‘kJSTypes’ defined but not used [-Werror=unused-variable] static Type* kJSTypes[] = {Type::Undefined(), Type::Null(), Type::Boolean(), ../../src/bignum.cc: In member function ‘void v8::internal::Bignum::AssignDecimalString(Vector<const char>)’: ../../src/bignum.cc:80:6: error: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Werror=strict-overflow] ../../src/compiler/ia32/code-generator-ia32.cc:1366:3: required from here ../../src/base/logging.h:123:26: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] DEFINE_CHECK_OP_IMPL(EQ, ==) BUG= Review URL: https://codereview.chromium.org/1371823002 Cr-Commit-Position: refs/heads/master@{#31095} PR-URL: nodejs/node#15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: GYP: Don't pass -Wno-format-pedantic to GCC. This flag is not understood correctly by GCC and breaks the GCC ARM and MIPS optdebug builds. Patch from Brendan Kirby <brendan.kirby@imgtec.com> BUG= Review URL: https://codereview.chromium.org/1369273003 Cr-Commit-Position: refs/heads/master@{#31013} PR-URL: nodejs/node#15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Allow ICU to normalize time zones There's at least one case of a time zone alias: Asia/Kathmandu aliases Asia/Katmandu. ICU seems to normalize to the (deprecated) latter choice. V8 internationalization choked on this change; this patch interprets ICU's output more precisely and allows it. BUG=chromium:487322 R=jungshik,adamk LOG=Y Review URL: https://codereview.chromium.org/1509273007 Cr-Commit-Position: refs/heads/master@{#32769} PR-URL: nodejs/node#15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

Original commit message: Timezone name check fix 1. Location names with more than one underscores (e.g. Ho_Chi_Minh) didn't work because of the way capturing works with repeated patterns in RE. It's now supported by changing the RE to capture the whole string and splitting on '_' in the next step. 2. Adds support for location names with a hyphen 3. Adds support for timezone ids with three parts (e.g. American/Argentina/Buenos_Aires) 4. Adds special handling of 'au', 'es' and 'of' in zone ids. They need to be kept in lowercase. (see the full list at https://en.wikipedia.org/wiki/List_of_tz_database_time_zones ) 5. Adds regression tests for all the above and make the existing tests more robust against future ICU changes. ICU canonicalizes zone names to deprecated names, but it may change. ( http://bugs.icu-project.org/trac/ticket/12044 ) BUG=364374 LOG=Y Review URL: https://codereview.chromium.org/1529363005 Cr-Commit-Position: refs/heads/master@{#33097} PR-URL: nodejs/node#15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Sep 22, 2017

MylesBorins requested a review from bnoordhuis September 22, 2017 18:51

Qantas94Heavy suggested changes Sep 23, 2017

View reviewed changes

jasnell approved these changes Sep 25, 2017

View reviewed changes

MylesBorins force-pushed the V8-icu-patch-4.x branch from 0eed431 to de847b7 Compare September 26, 2017 03:52

bnoordhuis requested changes Sep 26, 2017

View reviewed changes

MylesBorins force-pushed the V8-icu-patch-4.x branch from de847b7 to 9158d6c Compare September 27, 2017 17:40

bnoordhuis approved these changes Sep 27, 2017

View reviewed changes

hashseed approved these changes Sep 30, 2017

View reviewed changes

MylesBorins mentioned this pull request Oct 2, 2017

v4.8.5 proposal #15507

Closed

bnoordhuis approved these changes Oct 11, 2017

View reviewed changes

bnoordhuis and others added 3 commits October 11, 2017 14:29

build: fix make test-v8

875e396

MylesBorins force-pushed the V8-icu-patch-4.x branch from b99e874 to 0cc8102 Compare October 11, 2017 18:30

MylesBorins commented Oct 11, 2017

View reviewed changes

Qantas94Heavy approved these changes Oct 12, 2017

View reviewed changes

srl295 approved these changes Oct 12, 2017

View reviewed changes

MylesBorins pushed a commit that referenced this pull request Oct 18, 2017

build: fix make test-v8

3bd3418

PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

MylesBorins closed this Oct 18, 2017

MylesBorins pushed a commit that referenced this pull request Oct 25, 2017

build: fix make test-v8

e064ae6

PR-URL: #15562 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Steven R Loomis <srloomis@us.ibm.com>

MylesBorins mentioned this pull request Oct 25, 2017

v4.8.6 proposal #16500

Merged

MylesBorins deleted the V8-icu-patch-4.x branch November 14, 2017 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V8 icu patch 4.x #15562

V8 icu patch 4.x #15562

MylesBorins commented Sep 22, 2017

MylesBorins commented Sep 22, 2017 •

edited

Loading

MylesBorins commented Sep 22, 2017

Qantas94Heavy left a comment

Qantas94Heavy Sep 23, 2017 •

edited

Loading

Qantas94Heavy Sep 23, 2017

Qantas94Heavy Sep 23, 2017

jasnell left a comment

MylesBorins commented Sep 26, 2017 •

edited

Loading

bnoordhuis Sep 26, 2017

bnoordhuis Sep 26, 2017

MylesBorins commented Sep 27, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Sep 27, 2017

MylesBorins commented Sep 28, 2017 •

edited

Loading

MylesBorins commented Sep 29, 2017 •

edited

Loading

hashseed commented Sep 30, 2017

bnoordhuis commented Oct 2, 2017

MylesBorins commented Oct 2, 2017 •

edited

Loading

bnoordhuis commented Oct 2, 2017

MylesBorins commented Oct 2, 2017 •

edited

Loading

MylesBorins commented Oct 2, 2017 •

edited

Loading

MylesBorins commented Oct 11, 2017 •

edited

Loading

bnoordhuis Oct 11, 2017

MylesBorins Oct 11, 2017

MylesBorins Oct 11, 2017

bnoordhuis Oct 12, 2017

srl295 commented Oct 12, 2017

srl295 left a comment

MylesBorins commented Oct 18, 2017

V8 icu patch 4.x #15562

V8 icu patch 4.x #15562

Conversation

MylesBorins commented Sep 22, 2017

MylesBorins commented Sep 22, 2017 • edited Loading

MylesBorins commented Sep 22, 2017

Qantas94Heavy left a comment

Choose a reason for hiding this comment

Qantas94Heavy Sep 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

MylesBorins commented Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Sep 27, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Sep 28, 2017 • edited Loading

MylesBorins commented Sep 29, 2017 • edited Loading

hashseed commented Sep 30, 2017

bnoordhuis commented Oct 2, 2017

MylesBorins commented Oct 2, 2017 • edited Loading

bnoordhuis commented Oct 2, 2017

MylesBorins commented Oct 2, 2017 • edited Loading

MylesBorins commented Oct 2, 2017 • edited Loading

MylesBorins commented Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srl295 commented Oct 12, 2017

srl295 left a comment

Choose a reason for hiding this comment

MylesBorins commented Oct 18, 2017

MylesBorins commented Sep 22, 2017 •

edited

Loading

Qantas94Heavy Sep 23, 2017 •

edited

Loading

MylesBorins commented Sep 26, 2017 •

edited

Loading

MylesBorins commented Sep 27, 2017 •

edited

Loading

MylesBorins commented Sep 28, 2017 •

edited

Loading

MylesBorins commented Sep 29, 2017 •

edited

Loading

MylesBorins commented Oct 2, 2017 •

edited

Loading

MylesBorins commented Oct 2, 2017 •

edited

Loading

MylesBorins commented Oct 2, 2017 •

edited

Loading

MylesBorins commented Oct 11, 2017 •

edited

Loading