v8: fix new.target inheritance from c++ #9293

bnoordhuis · 2016-10-26T12:03:46Z

See #9288. Cherry-pick two commits from V8 5.2. The actual fix is commit v8/v8@73ee794 but it depends on commit v8/v8@306c412 which might be something of an ABI change. Opinions welcome on whether we should do this.

cc @nodejs/v8

bnoordhuis · 2016-10-26T12:05:33Z

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

I'll add a regression test if we decide to land this.

addaleax · 2016-10-26T15:27:23Z

deps/v8/include/v8.h

  internal::Object** implicit_args_;
  internal::Object** values_;
  int length_;
-  int is_construct_call_;


Yeah, removing this field will definitely break addons compiled for the existing versions of node v6. Maybe the changes can be patched up to duplicate information, so that is_construct_call_ is always populated with a correct value? (I can look into that myself but I wouldn’t want to make any promises about time.)

I'm in Germany for the next few days (Jena, in case you're wondering) so I probably won't have time to get back to it until next week.

It turned out to be a little more involved than just adding back the field because it's also set directly by generated code. I got it working locally and I'll push the changes shortly.

addaleax · 2016-10-27T14:15:39Z

deps/v8/src/objects.cc

-    case JS_DATE_TYPE:
-    case JS_ARRAY_TYPE:
-    case JS_MESSAGE_OBJECT_TYPE:
+    case JS_API_OBJECT_TYPE:


This line should probably be dropped? (Right now this breaks compiling a debug build, there is no JS_API_OBJECT_TYPE).

Good catch. Fixed, and I checked that the other fields do exist.

bnoordhuis · 2016-11-02T10:44:04Z

Updated, PTAL.

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

EDIT: Le sigh, the node-test-commit-v8-linux job still has infrastructural issues...

bnoordhuis · 2016-11-02T10:44:58Z

deps/v8/include/v8.h

@@ -3177,12 +3177,13 @@ class FunctionCallbackInfo {
                          Local<Function> Callee() const);
  V8_INLINE Local<Object> This() const;
  V8_INLINE Local<Object> Holder() const;
+  V8_INLINE Local<Value> NewTarget() const;


I can break out the new method into a separate commit in order to keep the meat of the pull request semver-patch.

nit: It might be a good idea to add the a DCHECK in IsConstructCall to make sure that the equivalence relationship between is_construct_call_ & 1 and !NewTarget->IsUndefined holds.

I see that a subsequent commit drops NewTarget. Feel free to ignore.

rvagg · 2016-11-02T10:56:36Z

Did an earlier version of this include the ABI breaking changes because I don't think I see it in the current incarnation, at least I don't see the is_construct_call stuff in v8.h here. Is it safe to assume this is ABI stable now?

I quite like the idea of keeping the NewTarget() stuff separate so we don't end up holding this off until we're prepared to do a semver-minor in v6 (it'd be preferable to hold that off for as long as possible). Do we get the fix for #9288 without that addition?

bnoordhuis · 2016-11-02T11:16:58Z

@rvagg Yes to both questions.

rvagg · 2016-11-02T11:23:01Z

OK then, my vote goes for splitting this into a semver-patch so we can get a fix out in the next v6 release and a semver-minor so we can complete the API at some point after. Thanks for the hard work of making this fit nicely @bnoordhuis.

@thealphanerd this'll be a good one to try your new ABI compatibility tester on.

mhdawson · 2016-11-04T21:11:53Z

v8 CI run -> https://ci.nodejs.org/job/node-test-commit-v8-linux/396/

MylesBorins · 2016-11-18T20:20:28Z

@bnoordhuis @nodejs/v8 do we want to try and get this in the next release?

outdated review

bnoordhuis · 2016-11-18T22:09:38Z

Updated with a commit that backs out NewTarget(). #9689 is the regression test, that should land in master first before it gets back-ported. I tested locally that the test fails with v6.x-staging and passes with this pull request applied.

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

bnoordhuis · 2016-11-18T22:22:39Z

+ gmake lint-ci
./node tools/jslint.js -j 2 -f tap -o test-eslint.tap \
    benchmark lib test tools
gmake: *** [Makefile:709: jslint-ci] Error 1

No clue as to why this PR would trigger a lint error, it only touches code in deps/v8...

bnoordhuis · 2016-11-18T22:32:40Z

Now with patches for s390, ppc and x87 applied. CI: https://ci.nodejs.org/job/node-test-pull-request/4902/

bnoordhuis · 2016-11-19T11:14:50Z

Rebased again: https://ci.nodejs.org/job/node-test-pull-request/4908/

Add a test that checks that new.target inheritance works when inheriting from a constructor defined in C++. PR-URL: nodejs#9689 Refs: nodejs#9288 Refs: nodejs#9293 Reviewed-By: Anna Henningsen <anna@addaleax.net>

bnoordhuis · 2016-11-21T13:40:49Z

Added the regression test from #9689. New CI: https://ci.nodejs.org/job/node-test-pull-request/4920/

MylesBorins · 2016-11-22T05:40:39Z

@bnoordhuis linting error is unrelated to this PR

bnoordhuis · 2017-02-08T15:02:24Z

V8 CI is brilliantly green, node CI is... eh, nothing that's caused by this PR.

@addaleax @nodejs/v8 Perhaps one of you can review the last commit?

ofrobots · 2017-02-08T17:51:10Z

deps/v8/src/s390/code-stubs-s390.cc

+  STATIC_ASSERT(FCA::kArgsLength == 8);
+
+  // new target
+  __ PushRoot(Heap::kUndefinedValueRootIndex);


For posterity, can you add a link to the commit where the s390 and x87 stubs come from.

ppc: v8/v8@4f75124
s390: v8/v8@38e7d1f
x87: v8/v8@1542e46

ofrobots · 2017-02-08T17:54:00Z

deps/v8/include/v8.h

@@ -3177,12 +3177,13 @@ class FunctionCallbackInfo {
                          Local<Function> Callee() const);
  V8_INLINE Local<Object> This() const;
  V8_INLINE Local<Object> Holder() const;
+  V8_INLINE Local<Value> NewTarget() const;


nit: It might be a good idea to add the a DCHECK in IsConstructCall to make sure that the equivalence relationship between is_construct_call_ & 1 and !NewTarget->IsUndefined holds.

ofrobots · 2017-02-08T17:57:22Z

deps/v8/include/v8.h

@@ -3177,12 +3177,13 @@ class FunctionCallbackInfo {
                          Local<Function> Callee() const);
  V8_INLINE Local<Object> This() const;
  V8_INLINE Local<Object> Holder() const;
+  V8_INLINE Local<Value> NewTarget() const;


I see that a subsequent commit drops NewTarget. Feel free to ignore.

bnoordhuis · 2017-02-13T18:41:21Z

Rebased and new CI runs:

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

MylesBorins

LGTM

bnoordhuis · 2017-02-27T16:16:02Z

@MylesBorins Should I land this or do you?

jasnell · 2017-02-27T19:04:57Z

@bnoordhuis ... I'm not @MylesBorins of course, but feel free to land in the staging branch :-)

Don't try to optimize known-unoptimizable functions when --always-opt is specified on the command line, it makes Crankshaft emit wrong code. This was fixed upstream when improved WASM support was introduced but that specific change can't be back-ported because it depends on prior work that doesn't exist in V8 5.1. Ergo, I decided to redo the fix from scratch. PR-URL: nodejs#9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

The patch has been modified to maintain ABI compatibility. The original change removes the v8::FunctionCallbackInfo<T>::is_construct_call_ field from deps/v8/include/v8.h. The field is set directly by JIT-ted code so the removal of those code paths has been backed out as well. Original commit message: [api] Expose FunctionCallbackInfo::NewTarget This is needed by Blink to implement the Custom Elements spec. BUG=v8:4261 LOG=y Review-Url: https://codereview.chromium.org/1910253005 Cr-Commit-Position: refs/heads/master@{nodejs#35833} Fixes: nodejs#9288 PR-URL: nodejs#9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Original commit message: When instantiating a subclassed API function, the instance cache is avoided. There is currently no direct API yet to instantiate a Template while passing in a new.target. It probably makes sense to extend ObjectTemplate::NewInstance to accept a new.target, in line with Reflect.construct. BUG=v8:3330, v8:5001 Review-Url: https://codereview.chromium.org/1972613002 Cr-Commit-Position: refs/heads/master@{nodejs#36179} Fixes: nodejs#9288 PR-URL: nodejs#9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Remove the new method that was introduced in the back-port of v8/v8@306c412c ("[api] Expose FunctionCallbackInfo::NewTarget") so that the meat of the patch can land in a patch release. This commit can be reverted again in the next minor release. Fixes: nodejs#9288 PR-URL: nodejs#9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

bnoordhuis · 2017-03-02T12:17:41Z

Landed in 1d631ce...2ebceed, cheers.

MylesBorins · 2017-03-03T02:00:48Z

@bnoordhuis is this able to be landed in the next release? The next release will be a patch and will not be able to include the revert that is mentioned above

bnoordhuis · 2017-03-03T08:24:24Z

@MylesBorins Yes, this PR is semver-patch, #11652 the semver-minor.

Don't try to optimize known-unoptimizable functions when --always-opt is specified on the command line, it makes Crankshaft emit wrong code. This was fixed upstream when improved WASM support was introduced but that specific change can't be back-ported because it depends on prior work that doesn't exist in V8 5.1. Ergo, I decided to redo the fix from scratch. PR-URL: #9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

The patch has been modified to maintain ABI compatibility. The original change removes the v8::FunctionCallbackInfo<T>::is_construct_call_ field from deps/v8/include/v8.h. The field is set directly by JIT-ted code so the removal of those code paths has been backed out as well. Original commit message: [api] Expose FunctionCallbackInfo::NewTarget This is needed by Blink to implement the Custom Elements spec. BUG=v8:4261 LOG=y Review-Url: https://codereview.chromium.org/1910253005 Cr-Commit-Position: refs/heads/master@{#35833} Fixes: #9288 PR-URL: #9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Original commit message: When instantiating a subclassed API function, the instance cache is avoided. There is currently no direct API yet to instantiate a Template while passing in a new.target. It probably makes sense to extend ObjectTemplate::NewInstance to accept a new.target, in line with Reflect.construct. BUG=v8:3330, v8:5001 Review-Url: https://codereview.chromium.org/1972613002 Cr-Commit-Position: refs/heads/master@{#36179} Fixes: #9288 PR-URL: #9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Remove the new method that was introduced in the back-port of v8/v8@306c412c ("[api] Expose FunctionCallbackInfo::NewTarget") so that the meat of the patch can land in a patch release. This commit can be reverted again in the next minor release. Fixes: #9288 PR-URL: #9293 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Add a test that checks that new.target inheritance works when inheriting from a constructor defined in C++. PR-URL: #9689 Refs: #9288 Refs: #9293 Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added the v6.x label Oct 26, 2016

bnoordhuis mentioned this pull request Oct 26, 2016

inheritance of C/C++ Addons doesn't work #9288

Closed

mscdex added v8 engine Issues and PRs related to the V8 dependency. addons Issues and PRs related to native addons. labels Oct 26, 2016

addaleax previously requested changes Oct 26, 2016

View reviewed changes

addaleax reviewed Oct 27, 2016

View reviewed changes

bnoordhuis force-pushed the fix9288 branch from 1d373dc to 071124a Compare November 2, 2016 10:41

bnoordhuis commented Nov 2, 2016

View reviewed changes

bnoordhuis mentioned this pull request Nov 18, 2016

test: add new.target add-on regression test #9689

Merged

bnoordhuis force-pushed the fix9288 branch from 071124a to 5a15cba Compare November 18, 2016 22:07

bnoordhuis force-pushed the fix9288 branch from 5a15cba to e791ec9 Compare November 18, 2016 22:31

MylesBorins force-pushed the v6.x-staging branch from 1e0a8d7 to ce999a6 Compare November 19, 2016 00:04

bnoordhuis force-pushed the fix9288 branch from e791ec9 to a4d3dec Compare November 19, 2016 11:13

ofrobots approved these changes Feb 8, 2017

View reviewed changes

bnoordhuis force-pushed the fix9288 branch from 6f1c1b8 to 406134c Compare February 13, 2017 18:39

jasnell approved these changes Feb 13, 2017

View reviewed changes

MylesBorins approved these changes Feb 21, 2017

View reviewed changes

MylesBorins force-pushed the v6.x-staging branch from c359018 to 1d631ce Compare February 22, 2017 02:08

bnoordhuis closed this Mar 2, 2017

bnoordhuis force-pushed the fix9288 branch from 406134c to 2ebceed Compare March 2, 2017 12:17

bnoordhuis merged commit 2ebceed into nodejs:v6.x-staging Mar 2, 2017

bnoordhuis deleted the fix9288 branch March 2, 2017 12:17

addaleax mentioned this pull request Mar 2, 2017

Revert "v8: drop v8::FunctionCallbackInfo<T>::NewTarget()" #11652

Closed

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

mscdex mentioned this pull request Mar 21, 2017

V8 Fatal Exception, 0x99b617 #11977

Closed

jasongin mentioned this pull request Mar 26, 2017

Consider "new.target" for constructor callbacks nodejs/abi-stable-node#200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v8: fix new.target inheritance from c++ #9293

v8: fix new.target inheritance from c++ #9293

bnoordhuis commented Oct 26, 2016

bnoordhuis commented Oct 26, 2016

addaleax Oct 26, 2016

bnoordhuis Oct 26, 2016

bnoordhuis Nov 2, 2016

addaleax Oct 27, 2016

bnoordhuis Nov 2, 2016

bnoordhuis commented Nov 2, 2016 •

edited

Loading

bnoordhuis Nov 2, 2016

ofrobots Feb 8, 2017

ofrobots Feb 8, 2017

rvagg commented Nov 2, 2016

bnoordhuis commented Nov 2, 2016

rvagg commented Nov 2, 2016

mhdawson commented Nov 4, 2016

MylesBorins commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 19, 2016

bnoordhuis commented Nov 21, 2016

MylesBorins commented Nov 22, 2016

bnoordhuis commented Feb 8, 2017

ofrobots Feb 8, 2017

jbajwa Feb 8, 2017

ofrobots Feb 8, 2017

ofrobots Feb 8, 2017

bnoordhuis commented Feb 13, 2017

MylesBorins left a comment

bnoordhuis commented Feb 27, 2017

jasnell commented Feb 27, 2017

bnoordhuis commented Mar 2, 2017

MylesBorins commented Mar 3, 2017

bnoordhuis commented Mar 3, 2017

v8: fix new.target inheritance from c++ #9293

v8: fix new.target inheritance from c++ #9293

Conversation

bnoordhuis commented Oct 26, 2016

bnoordhuis commented Oct 26, 2016

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

Choose a reason for hiding this comment

bnoordhuis commented Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented Nov 2, 2016

bnoordhuis commented Nov 2, 2016

rvagg commented Nov 2, 2016

mhdawson commented Nov 4, 2016

MylesBorins commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 18, 2016

bnoordhuis commented Nov 19, 2016

bnoordhuis commented Nov 21, 2016

MylesBorins commented Nov 22, 2016

bnoordhuis commented Feb 8, 2017

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

bnoordhuis commented Feb 13, 2017

MylesBorins left a comment

Choose a reason for hiding this comment

bnoordhuis commented Feb 27, 2017

jasnell commented Feb 27, 2017

bnoordhuis commented Mar 2, 2017

MylesBorins commented Mar 3, 2017

bnoordhuis commented Mar 3, 2017

bnoordhuis commented Nov 2, 2016 •

edited

Loading