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: V8: backport 4263f8a5e8e0 #35650

Closed
wants to merge 2 commits into from

Conversation

bdougie
Copy link
Contributor

@bdougie bdougie commented Oct 14, 2020

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ä <[email protected]>
Commit-Queue: Gus Caplan <[email protected]>
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 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
Copy link
Contributor Author

bdougie commented Oct 14, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 14, 2020

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

some tiny issues with conflict resolution

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 14, 2020

@MylesBorins
Copy link
Contributor

@devsnek any idea why the V8 tests are failing?

@bdougie
Copy link
Contributor Author

bdougie commented Oct 15, 2020

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
Copy link
Contributor

/cc @nodejs/v8-update

@Trott
Copy link
Member

Trott commented Oct 21, 2020

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 Trott force-pushed the bdougie-v8-backport branch 2 times, most recently from 678b58e to eb1aac1 Compare October 21, 2020 13:11
@Trott
Copy link
Member

Trott commented Oct 21, 2020

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if we can get the V8 CI to pass

@codecov-io

This comment has been minimized.

@MylesBorins
Copy link
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
Copy link
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
Copy link
Member

joyeecheung commented Oct 21, 2020

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

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

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ä <[email protected]>
    Commit-Queue: Gus Caplan <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 21, 2020

@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
Copy link
Member

joyeecheung commented Oct 21, 2020

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
Copy link
Contributor

MylesBorins commented Oct 21, 2020

@MylesBorins MylesBorins added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020
@github-actions 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
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ä <[email protected]>
    Commit-Queue: Gus Caplan <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

PR-URL: #35650
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
targos pushed a commit that referenced this pull request Nov 3, 2020
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ä <[email protected]>
    Commit-Queue: Gus Caplan <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#70099}

Refs: v8/v8@4263f8a

PR-URL: #35650
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@targos targos mentioned this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants