Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cherry-pick patch from V8 upstream that fixes instanceof problem #7638

Closed
wants to merge 5 commits into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Jul 9, 2016

Checklist
  • make -j4 test (UNIX)
  • test is included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps V8

Description of change

Float the following patch:
InstanceOfStub incorrectly interprets the hole as a prototype.
https://codereview.chromium.org/1810953002/

Fixes: #7592

/cc @bnoordhuis

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 9, 2016
@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 9, 2016
@addaleax
Copy link
Member

addaleax commented Jul 9, 2016

I think you may want to switch the order of the commits here, so that there is no point in the history in which make test fails?

@fhinkel
Copy link
Member Author

fhinkel commented Jul 9, 2016

Oh, you're totally right. Done.

@addaleax addaleax added the v6.x label Jul 9, 2016
@@ -0,0 +1,7 @@
'use strict';
require('../common');
var assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here and below?

@bnoordhuis
Copy link
Member

@fhinkel Can you reword the commit log to be more like (e.g.) b88dee2? We use 'cherry-pick' for a clean cherry-pick and 'backport' if the patch needed nudging.

(At least, I do. Possibly some unwritten rule that we need to write down somewhere.)

@fhinkel fhinkel changed the title Float V8 patch that fixes instanceof problem Cherry-pick patch from V8 upstream that fixes instanceof problem Jul 10, 2016
@fhinkel
Copy link
Member Author

fhinkel commented Jul 10, 2016

I changed the commit message and the test. @bnoordhuis can you have another look, please?

@addaleax
Copy link
Member

LGTM

@MylesBorins
Copy link
Contributor

@bnoordhuis I started playing with making a backport / cherry-pick guide. Is there any other "unwritten rules" you can think of?

@bnoordhuis
Copy link
Member

bnoordhuis commented Jul 11, 2016

@thealphanerd "Run the V8 and node.js test suite on the CI". That's about it.

EDIT: "Don't forget to bump the patchlevel..."

@Fishrock123
Copy link
Contributor

FreeBSD has an 'interesting' error:

ln -fs out/Release/node node
./node: not found
Makefile:158: recipe for target 'test/addons/.buildstamp' failed
gmake[1]: *** [test/addons/.buildstamp] Error 1
gmake[1]: *** Waiting for unfinished jobs....
gmake[1]: Leaving directory '/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64'
Makefile:321: recipe for target 'run-ci' failed
gmake: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

Not sure what's up with that.

Looks like ppc little-endian ubuntu is failing though, and the v8 CI also seems to have some failures.

not ok 579 parallel/test-instanceof
# 
# assert.js:89
# throw new assert.AssertionError({
# ^
# AssertionError: false == true
# at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-plinux/nodes/ppcbe-ubuntu1404/test/parallel/test-instanceof.js:7:1)
# at Module._compile (module.js:541:32)
# at Object.Module._extensions..js (module.js:550:10)
# at Module.load (module.js:458:32)
# at tryModuleLoad (module.js:417:12)
# at Function.Module._load (module.js:409:3)
# at Module.runMain (module.js:575:10)
# at run (bootstrap_node.js:352:7)
# at startup (bootstrap_node.js:144:9)
# at bootstrap_node.js:467:3

@bnoordhuis
Copy link
Member

I think it's because it's missing the PPC changes from v8/v8@3a903c4.

@mhdawson
Copy link
Member

Quite possible that there are similar changes for s390 as well. Will look at adding s390 to the v8 test job as well.

@bnoordhuis
Copy link
Member

It doesn't matter for this particular patch (no s390 in v6) but it would be good in general.

@fhinkel
Copy link
Member Author

fhinkel commented Jul 11, 2016

Thanks for running the CI. I'll add the PPC and X87 patches.

@fhinkel
Copy link
Member Author

fhinkel commented Jul 11, 2016

I cherry-picked the patches for the remaining architectures (not for s390 because it's not in v6). Can you have another look, please?

@addaleax
Copy link
Member

@fhinkel
Copy link
Member Author

fhinkel commented Jul 11, 2016

Do I need to bump the patch level or are we only doing that for 5.1?

@bnoordhuis
Copy link
Member

Yes, please bump the patchlevel.

@fhinkel fhinkel force-pushed the i7592 branch 2 times, most recently from 92345b7 to 6657bd3 Compare July 11, 2016 19:45
@fhinkel
Copy link
Member Author

fhinkel commented Jul 11, 2016

Bumped and rebased, so test is added after cherry-picks.

@ofrobots
Copy link
Contributor

LGTM.

evanlucas pushed a commit that referenced this pull request Jul 21, 2016
Original commit message:

    InstanceOfStub incorrectly interprets the hole as a prototype.

    Repair this to match what the runtime correctly does, by first
    checking if the function is a constructor before we access the
    prototype.

    [email protected]

    BUG=

    Committed: https://crrev.com/2aa070be4fd2960df98905b254f12ed801ef26cd

    Cr-Commit-Position: refs/heads/master@{#34863}

This fixes the behavior of instanceof when the second parameter is not a
constructor.

Fixes: #7592
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 21, 2016
Original commit message:

    PPC: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected], [email protected],
    [email protected]

    BUG=

    Review URL: https://codereview.chromium.org/1811013002

    Cr-Commit-Position: refs/heads/master@{#34869}

Fixes: #7592 for PPC
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 21, 2016
Original commit message:

    port 2aa070b (r34863)

    original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    BUG=

    Review URL: https://codereview.chromium.org/1809333002

    Cr-Commit-Position: refs/heads/master@{#34880}

Fixes: #7592 for X87
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 21, 2016
Ref: #7592.
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 21, 2016
PR-URL: #7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
evanlucas added a commit that referenced this pull request Jul 21, 2016
Notable changes:

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) #7638
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641

PR-URL: #7782
@evanlucas
Copy link
Contributor

landed in v6.x in 71f84b5...164981a. Thanks!

@evanlucas evanlucas closed this Jul 21, 2016
evanlucas added a commit that referenced this pull request Jul 21, 2016
Notable changes:

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) #7638
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641

PR-URL: #7782
@bnoordhuis
Copy link
Member

I would have preferred it if this PR had landed as a single commit. We now have commits in our history where the build is known broken on some architectures, making things like git-bisect more difficult.

@fhinkel
Copy link
Member Author

fhinkel commented Jul 22, 2016

@bnoordhuis I'm not sure I understand your concern. The test is added after the three cherry-picks, I don't think the build is broken in any of the revisions.

Should bumping the patch level be a separate commit or squashed? @thealphanerd where's your cherry-pick guide?

@bnoordhuis
Copy link
Member

Sorry, I must have been thinking of another PR. You're right, here it was just the test that was failing without the PPC changes.

I have a preference for bumping the patch level in the same commit. It makes it an atomic unit, easy to roll back.

BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Jul 22, 2016
Original commit message:

    InstanceOfStub incorrectly interprets the hole as a prototype.

    Repair this to match what the runtime correctly does, by first
    checking if the function is a constructor before we access the
    prototype.

    [email protected]

    BUG=

    Committed: https://crrev.com/2aa070be4fd2960df98905b254f12ed801ef26cd

    Cr-Commit-Position: refs/heads/master@{#34863}

This fixes the behavior of instanceof when the second parameter is not a
constructor.

Fixes: nodejs/node#7592
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Jul 22, 2016
Original commit message:

    PPC: InstanceOfStub incorrectly interprets the hole as a prototype.
    Port 2aa070b

    Original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    [email protected], [email protected], [email protected],
    [email protected]

    BUG=

    Review URL: https://codereview.chromium.org/1811013002

    Cr-Commit-Position: refs/heads/master@{#34869}

Fixes: nodejs/node#7592 for PPC
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Jul 22, 2016
Original commit message:

    port 2aa070b (r34863)

    original commit message:
        Repair this to match what the runtime correctly does,
        by first checking if the function is a constructor
        before we access the prototype.

    BUG=

    Review URL: https://codereview.chromium.org/1809333002

    Cr-Commit-Position: refs/heads/master@{#34880}

Fixes: nodejs/node#7592 for X87
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Jul 22, 2016
PR-URL: nodejs/node#7638
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
lukesampson pushed a commit to ScoopInstaller/Scoop that referenced this pull request Jul 24, 2016
### Notable changes

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) [#7602](nodejs/node#7602)
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) [#7176](nodejs/node#7176)
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) [#7531](nodejs/node#7531)
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) [#7638](nodejs/node#7638)
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) [#7794](nodejs/node#7794)
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) [#7641](nodejs/node#7641)
@rvagg
Copy link
Member

rvagg commented Oct 19, 2016

@fhinkel do you think this should also land on master? it's dying out with the v6.x series but perhaps it's generic enough to warrant keeping around.

@fhinkel
Copy link
Member Author

fhinkel commented Oct 19, 2016

@rvagg You mean if the test should be ported to master? Yes, let me do that. Then we can let this die with v6.x.

@rvagg
Copy link
Member

rvagg commented Oct 20, 2016

@fhinkel yeah, thanks, it came up as one of the commits on v6.x that are not on v7.x and I just wanted clarification regarding whether this should live on beyond v6.x and getting it on master is obviously the way to do that.

fhinkel added a commit to fhinkel/node that referenced this pull request Oct 20, 2016
Add regression test for issue nodejs#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: nodejs#7638 and
nodejs#7592.
fhinkel added a commit that referenced this pull request Oct 20, 2016
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 20, 2016
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 15, 2016
Add regression test for issue
#7592.
The issue was fixed in upstream V8 and this test case was
previously added with a manual cherry pick for v6.x.

Related to: #7638 and
#7592.

PR-URL: #9178
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants