FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: V8: backport 4263f8a5e8e0 by bdougie · Pull Request #35650 · nodejs/node · GitHub

/ node Public

deps: V8: backport 4263f8a5e8e0 - #35650

Closed
bdougie wants to merge 2 commits into
nodejs:masterfrom
bdougie:bdougie-v8-backport
ConversationCommits (2)ChecksFiles changed
Closed

deps: V8: backport 4263f8a5e8e0#35650
bdougie wants to merge 2 commits into
nodejs:masterfrom
bdougie:bdougie-v8-backport

Conversation

bdougie commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

Original commit message:

parser: better error message for await+tla

Bug: v8:9344, v8:6513
Change-Id: I1854e483515e7da99192367b6764a0ec7c8b41d9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2411687
Reviewed-by: Marja Hölttä <marja@chromium.org>
Commit-Queue: Gus Caplan <snek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Oct 14, 2020

bdougie commented Oct 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

re: #35196 (comment)

I originally opened the above PR updating the error messages. That was instead landed upstream. This backport adds those changes and closes out #35196

MylesBorins mentioned this pull request Oct 14, 2020
Closed
4 tasks

nodejs-github-bot commented Oct 14, 2020 •
edited by MylesBorins
Loading

Copy link
Copy Markdown
Collaborator

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

MylesBorins reviewed Oct 14, 2020
View reviewed changes

MylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

some tiny issues with conflict resolution

Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden
Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden
Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden Outdated
Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden Outdated
Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden Outdated
Comment thread deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden Outdated

nodejs-github-bot commented Oct 14, 2020 •
edited by MylesBorins
Loading

Copy link
Copy Markdown
Collaborator

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

MylesBorins commented Oct 15, 2020

Copy link
Copy Markdown
Contributor

@devsnek any idea why the V8 tests are failing?

bdougie commented Oct 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The failures are in files that I touched (fixed merge conflicts). Curious if it is a white space thing or similar.

:53: Uncaught TypeError: '#b' was defined without a setter
:46: Uncaught TypeError: '#c' was defined without a getter
:61: Uncaught TypeError: '#d' was defined without a setter
:46: Uncaught TypeError: '#e' was defined without a getter
stderr:
Generated has extra lines after line 182
Generated: ''

MylesBorins commented Oct 16, 2020

Copy link
Copy Markdown
Contributor

/cc @nodejs/v8-update

Trott commented Oct 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

I believe V8-CI skips rebasing by default. I ran it again, but this time rebasing to see if that fixed things. https://ci.nodejs.org/job/node-test-commit-v8-linux/3493/ It reported that it couldn't rebase, which is a known issue in our CI with merge commits. I'll force-push out the merge commit from @MylesBorins and try again...

Trott force-pushed the bdougie-v8-backport branch 2 times, most recently from 678b58e to eb1aac1 Compare October 21, 2020 13:11

Trott commented Oct 21, 2020

Copy link
Copy Markdown
Member

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/3494/

Trott approved these changes Oct 21, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if we can get the V8 CI to pass

This comment has been minimized.

Sign in to view

MylesBorins commented Oct 21, 2020

Copy link
Copy Markdown
Contributor

@Trott the issue with CI is that the .golden files need to be regenerated and afaict there is no easy way for us to do this. @devsnek was looking into this, and I believe chatted with @joyeecheung about it to.

Maybe someone from @nodejs/v8-update would know how to help.

joyeecheung commented Oct 21, 2020

Copy link
Copy Markdown
Member

The failures are in files that I touched (fixed merge conflicts). Curious if it is a white space thing or similar.

Yes you need to remove the whitespace changes.

joyeecheung commented Oct 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Looks like the CI jobs are still failing because of git issues

stderr: error: you need to resolve your current index first

deps: V8: backport 4263f8a5e8e0
ff9c811 
Original commit message:

    parser: better error message for await+tla

    Bug: v8:9344, v8:6513
    Change-Id: I1854e483515e7da99192367b6764a0ec7c8b41d9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2411687
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Gus Caplan <snek@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

MylesBorins commented Oct 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

@joyeecheung it looks like the updates to the golden files from the original CL got lost when we updated V8 on master. I've re-applied the changes

edit:

Another crack at V8-CI https://ci.nodejs.org/job/node-test-commit-v8-linux/3495/

joyeecheung commented Oct 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

I did a rebaseline locally and this is the patch I get (compared to the state after this force push) and it passes the cctest locally. Should I update this branch?

See diff 
diff --git a/deps/v8/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden b/deps/v8/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden
index 5ce6e397..6d5bb4a5 100644
--- a/deps/v8/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden
+++ b/deps/v8/test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden
@@ -25,7 +25,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(18),
-                B(Wide), B(LdaSmi), I16(262),
+                B(Wide), B(LdaSmi), I16(263),
                 B(Star), R(2),
                 B(LdaConstant), U8(0),
                 B(Star), R(3),
@@ -56,7 +56,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(264),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(265),
                 B(Star), R(0),
                 B(LdaConstant), U8(0),
                 B(Star), R(1),
@@ -83,7 +83,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(264),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(265),
                 B(Star), R(0),
                 B(LdaConstant), U8(0),
                 B(Star), R(1),
@@ -122,7 +122,7 @@ bytecodes: [
   /*   94 E> */ B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(18),
-                B(Wide), B(LdaSmi), I16(262),
+                B(Wide), B(LdaSmi), I16(263),
                 B(Star), R(2),
                 B(LdaConstant), U8(0),
                 B(Star), R(3),
@@ -144,7 +144,7 @@ bytecodes: [
   /*  109 E> */ B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(18),
-                B(Wide), B(LdaSmi), I16(263),
+                B(Wide), B(LdaSmi), I16(264),
                 B(Star), R(3),
                 B(LdaConstant), U8(0),
                 B(Star), R(4),
@@ -159,7 +159,7 @@ bytecodes: [
   /*  133 E> */ B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(18),
-                B(Wide), B(LdaSmi), I16(262),
+                B(Wide), B(LdaSmi), I16(263),
                 B(Star), R(2),
                 B(LdaConstant), U8(0),
                 B(Star), R(3),
@@ -189,7 +189,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(266),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(267),
                 B(Star), R(0),
                 B(LdaConstant), U8(0),
                 B(Star), R(1),
@@ -215,7 +215,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(265),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(266),
                 B(Star), R(0),
                 B(LdaConstant), U8(0),
                 B(Star), R(1),
@@ -241,7 +241,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(266),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(267),
                 B(Star), R(0),
                 B(LdaConstant), U8(0),
                 B(Star), R(1),
@@ -267,7 +267,7 @@ frame size: 3
 parameter count: 1
 bytecode array length: 16
 bytecodes: [
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(265),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(266),
                 B(Star), R(1),
                 B(LdaConstant), U8(0),
                 B(Star), R(2),

MylesBorins commented Oct 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Another V8 CI https://ci.nodejs.org/job/node-test-commit-v8-linux/3501/

MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020
MylesBorins pushed a commit that referenced this pull request Oct 21, 2020
deps: V8: backport 4263f8a5e8e0
db4ce85 
Original commit message:

    parser: better error message for await+tla

    Bug: v8:9344, v8:6513
    Change-Id: I1854e483515e7da99192367b6764a0ec7c8b41d9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2411687
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Gus Caplan <snek@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

PR-URL: #35650
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2020
deps: V8: backport 4263f8a5e8e0
894419c 
Original commit message:

    parser: better error message for await+tla

    Bug: v8:9344, v8:6513
    Change-Id: I1854e483515e7da99192367b6764a0ec7c8b41d9
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2411687
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Gus Caplan <snek@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

PR-URL: #35650
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
targos mentioned this pull request Nov 3, 2020
Merged
This was referenced Aug 16, 2022
Open
Open
Dmarch28 mentioned this pull request Aug 16, 2022
Open
wedataintelligence mentioned this pull request Aug 16, 2022
Open
kissofire mentioned this pull request Aug 16, 2022
Open
molllyn1 mentioned this pull request Aug 17, 2022
Open
snyk-bot mentioned this pull request Aug 17, 2022
Open
baby636 mentioned this pull request Aug 17, 2022
Open
ron-a20 mentioned this pull request Aug 17, 2022
Open
snyk-bot mentioned this pull request Sep 25, 2022
Open
wedataintelligence mentioned this pull request Sep 26, 2022
Open
137717unity mentioned this pull request Sep 26, 2022
Open
snyk-bot mentioned this pull request Sep 27, 2022
Open
wedataintelligence mentioned this pull request Sep 29, 2022
Open
This was referenced Sep 29, 2022
Open
Open
This was referenced Oct 5, 2022
Open
Open
snyk-bot mentioned this pull request Oct 6, 2022
Open
137717unity mentioned this pull request Oct 7, 2022
Open
wedataintelligence mentioned this pull request Oct 7, 2022
Open
This was referenced Oct 8, 2022
Open
Open
137717unity mentioned this pull request Oct 9, 2022
Open
snyk-bot mentioned this pull request Oct 9, 2022
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung left review comments
Trott Trott approved these changes
+1 more reviewer
MylesBorins MylesBorins approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL