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

deps: backport 7dfb5beeec from V8 upstream #7348

Closed
wants to merge 1 commit into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Jun 21, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons typeof null ==="undefined" is returning
true instead of false.

Original commit message:

Fix 'typeof null' canonicalization in crankshaft

BUG=

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

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

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 21, 2016
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jun 21, 2016

@indutny
Copy link
Member

indutny commented Jun 21, 2016

Any reason to rush it and not wait until it will be merged into V8 5.2?

@MylesBorins
Copy link
Contributor Author

Based on the reddit thread it seemed like people were running into this problem in production. We can likely wait on it, but I wasn't sure how long it would take to prove out that the V8 5.2 update is non ABI breaking

@indutny
Copy link
Member

indutny commented Jun 21, 2016

@thealphanerd 👍 then.

@bnoordhuis
Copy link
Member

@thealphanerd Can you request a back-port to 5.0 and 5.1 upstream? Then we can just upgrade wholesale.

@bnoordhuis
Copy link
Member

Also, it looks like v6 is affected but v4 and v5 are not.

@MylesBorins
Copy link
Contributor Author

@bnoordhuis more than happy to. What is the process for doing so?

@bnoordhuis
Copy link
Member

@MylesBorins
Copy link
Contributor Author

I have opened a backport request in the chromium tracker

https://codereview.chromium.org/2087803004/

@ofrobots seems to imply that we should likely float for V8 5 until nodejs/Release#111 has been sorted

@ofrobots
Copy link
Contributor

This got cherry-picked onto in V8 5.1 earlier today. As far as V8 5.0 is concerned, that branch is no longer getting tested, and there is no official process to merge changes into it, until nodejs/Release#111 gets to closure. /cc @natorion

Until then I think we should float this patch.

@thealphanerd instead if the reddit link, could you link to the upstream bug for this issue: https://bugs.chromium.org/p/chromium/issues/detail?id=604033.

Otherwise this LGTM as long as V8 tests pass.

@bnoordhuis
Copy link
Member

LGTM but I second Ali's comment about the reddit link.

@MylesBorins
Copy link
Contributor Author

Link updated. In future I'll double check the chromium tracker first 😄

V8 tests passed, just waiting on a fresh run of the CI

@targos
Copy link
Member

targos commented Jun 21, 2016

LGTM.
There is a nit in the commit message: s/B8/V8/

This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

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

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

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
@MylesBorins
Copy link
Contributor Author

thanks @targos I was doing s/v/V and accidentally s/v/B'd

¯_(ツ)_/¯

@MylesBorins MylesBorins changed the title deps: backport 7dfb5beeec from v8 upstream deps: backport 7dfb5beeec from V8 upstream Jun 21, 2016
@MylesBorins
Copy link
Contributor Author

landed in a84b0f2

MylesBorins added a commit that referenced this pull request Jun 22, 2016
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

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

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

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: #7348
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins added a commit that referenced this pull request Jun 22, 2016
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

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

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

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: #7348
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins
Copy link
Contributor Author

I've gone ahead and landed this on v6.x as c544213

@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Jul 7, 2016
This commit backports a fix to a JIT bug in V8.
After 100 or so comparisons `typeof null ==="undefined"` is returning
`true` instead of `false`.

Original commit message:

	Fix 'typeof null' canonicalization in crankshaft

	BUG=

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

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

Ref: https://bugs.chromium.org/p/chromium/issues/detail?id=604033
PR-URL: nodejs/node#7348
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@MylesBorins MylesBorins deleted the null-check branch July 8, 2016 14:53
@MylesBorins MylesBorins removed their assignment Dec 27, 2016
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.

6 participants