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: patch V8 to 8.1.307.26 #32521

Closed
wants to merge 3 commits into from
Closed

Conversation

mmarchini
Copy link
Contributor

Refs: v8/v8@8.1.307.20...8.1.307.26

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. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Mar 27, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 27, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30133/ (:heavy_check_mark:)

@mmarchini
Copy link
Contributor Author

@targos I had to revert a commit from before the V8 8.1 upgrade, because git node v8 minor couldn't apply the patch. I thought git node v8 major would replace the entire deps/v8 folder when we do a major bump. Isn't that the case?

@targos
Copy link
Member

targos commented Mar 27, 2020

I thought git node v8 major would replace the entire deps/v8 folder when we do a major bump. Isn't that the case?

It is. deps/v8 is removed entirely in that process.

@targos
Copy link
Member

targos commented Mar 27, 2020

@mmarchini did you use git node land to land V8 8.1 on master? Maybe it happened because of that.
I avoid using it for V8 updates because I already had trouble with it before.

@mmarchini
Copy link
Contributor Author

I used git node land. This was likely the only stray commit though, but I'll double-check later.

@mmarchini
Copy link
Contributor Author

mmarchini commented Mar 30, 2020

A diff of deps/v8 between the landed commit and the PR commit doesn't show any meaningful difference, so I don't think it was git land. Not sure what happened here. The only differences were whitespace changes on some V8 test fixtures, which I guess is fine?

I also ran a diff between deps/v8 and a clean v8 checkout (excluding files handled by DEPS):

$ diff -X <(echo "*.git" && echo ".gitignore" && echo "third_party*" && echo "trace_event_common.h") -Naur deps/v8 /tmp/v8
diff -X /proc/self/fd/11 -Naur deps/v8/src/parsing/parser.cc /tmp/v8/src/parsing/parser.cc
--- deps/v8/src/parsing/parser.cc	2020-03-30 10:16:35.917781551 -0700
+++ /tmp/v8/src/parsing/parser.cc	2020-03-30 10:15:50.779938045 -0700
@@ -505,6 +505,7 @@
                         Scope::DeserializationMode::kIncludingVariables);
 
   scanner_.Initialize();
+  scanner_.SkipHashBang();
   FunctionLiteral* result = DoParseProgram(isolate, info);
   MaybeResetCharacterStream(info, result);
   MaybeProcessSourceRanges(info, result, stack_limit_);
diff -X /proc/self/fd/11 -Naur deps/v8/src/parsing/preparser.cc /tmp/v8/src/parsing/preparser.cc
--- deps/v8/src/parsing/preparser.cc	2020-03-30 10:16:35.917781551 -0700
+++ /tmp/v8/src/parsing/preparser.cc	2020-03-30 10:15:50.779938045 -0700
@@ -75,6 +75,10 @@
   scope->set_is_being_lazily_parsed(true);
 #endif
 
+  // Note: We should only skip the hashbang in non-Eval scripts
+  // (currently, Eval is not handled by the PreParser).
+  scanner()->SkipHashBang();
+
   // ModuleDeclarationInstantiation for Source Text Module Records creates a
   // new Module Environment Record whose outer lexical environment record is
   // the global scope.
diff -X /proc/self/fd/11 -Naur deps/v8/src/parsing/scanner.cc /tmp/v8/src/parsing/scanner.cc
--- deps/v8/src/parsing/scanner.cc	2020-03-30 10:16:35.917781551 -0700
+++ /tmp/v8/src/parsing/scanner.cc	2020-03-30 10:15:50.779938045 -0700
@@ -314,6 +314,13 @@
   return Token::ILLEGAL;
 }
 
+void Scanner::SkipHashBang() {
+  if (c0_ == '#' && Peek() == '!' && source_pos() == 0) {
+    SkipSingleLineComment();
+    Scan();
+  }
+}
+
 Token::Value Scanner::ScanHtmlComment() {
   // Check for <!-- comments.
   DCHECK_EQ(c0_, '!');
diff -X /proc/self/fd/11 -Naur deps/v8/src/parsing/scanner.h /tmp/v8/src/parsing/scanner.h
--- deps/v8/src/parsing/scanner.h	2020-03-30 10:16:35.917781551 -0700
+++ /tmp/v8/src/parsing/scanner.h	2020-03-30 10:15:50.779938045 -0700
@@ -422,6 +422,9 @@
 
   const Utf16CharacterStream* stream() const { return source_; }
 
+  // If the next characters in the stream are "#!", the line is skipped.
+  void SkipHashBang();
+
  private:
   // Scoped helper for saving & restoring scanner error state.
   // This is used for tagged template literals, in which normally forbidden
diff -X /proc/self/fd/11 -Naur deps/v8/src/parsing/scanner-inl.h /tmp/v8/src/parsing/scanner-inl.h
--- deps/v8/src/parsing/scanner-inl.h	2020-03-30 10:16:35.917781551 -0700
+++ /tmp/v8/src/parsing/scanner-inl.h	2020-03-30 10:15:50.779938045 -0700
@@ -506,10 +506,6 @@
           return ScanTemplateSpan();
 
         case Token::PRIVATE_NAME:
-          if (source_pos() == 0 && Peek() == '!') {
-            token = SkipSingleLineComment();
-            continue;
-          }
           return ScanPrivateName();
 
         case Token::WHITESPACE:
diff -X /proc/self/fd/11 -Naur deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden /tmp/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden
--- deps/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden	2020-03-30 10:16:36.029786139 -0700
+++ /tmp/v8/test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden	2020-03-30 10:15:50.867941628 -0700
@@ -111,7 +111,7 @@
     #d() { return 1; }
     constructor() { (() => this)().#d(); }
   }
-
+  
   var test = D;
   new test;
 "
@@ -147,3 +147,4 @@
 ]
 handlers: [
 ]
+
diff -X /proc/self/fd/11 -Naur deps/v8/test/inspector/debugger/class-private-methods-preview-expected.txt /tmp/v8/test/inspector/debugger/class-private-methods-preview-expected.txt
--- deps/v8/test/inspector/debugger/class-private-methods-preview-expected.txt	2020-03-30 10:16:36.073787942 -0700
+++ /tmp/v8/test/inspector/debugger/class-private-methods-preview-expected.txt	2020-03-30 10:15:50.899942930 -0700
@@ -9,7 +9,7 @@
     [0] : {
         name : #method
         type : function
-        value :
+        value : 
     }
     [1] : {
         name : #accessor
@@ -21,7 +21,7 @@
     [0] : {
         name : #method
         type : function
-        value :
+        value : 
     }
     [1] : {
         name : #accessor
diff -X /proc/self/fd/11 -Naur deps/v8/test/inspector/debugger/wasm-scope-info-liftoff-expected.txt /tmp/v8/test/inspector/debugger/wasm-scope-info-liftoff-expected.txt
--- deps/v8/test/inspector/debugger/wasm-scope-info-liftoff-expected.txt	2020-03-30 10:16:36.077788106 -0700
+++ /tmp/v8/test/inspector/debugger/wasm-scope-info-liftoff-expected.txt	2020-03-30 10:15:50.903943093 -0700
@@ -17,7 +17,7 @@
    globals: "global#0": 0 (number)
  - scope (local):
    locals: "i32_arg": 42 (number), "i32_local": 0 (number)
-   stack:
+   stack: 
 at B (liftoff) (0:76):
  - scope (global):
    globals: "global#0": 0 (number)
@@ -29,7 +29,7 @@
    globals: "global#0": 0 (number)
  - scope (local):
    locals: "arg#0": 42 (number)
-   stack:
+   stack: 
 at (anonymous) (0:17):
  - scope (global):
    -- skipped globals
@@ -54,7 +54,7 @@
    globals: "global#0": 0 (number)
  - scope (local):
    locals: "arg#0": 42 (number)
-   stack:
+   stack: 
 at (anonymous) (0:17):
  - scope (global):
    -- skipped globals
@@ -67,7 +67,7 @@
    globals: "global#0": 42 (number)
  - scope (local):
    locals: "i32_arg": 42 (number), "i32_local": 0 (number)
-   stack:
+   stack: 
 at B (liftoff) (0:76):
  - scope (global):
    globals: "global#0": 42 (number)
@@ -79,7 +79,7 @@
    globals: "global#0": 42 (number)
  - scope (local):
    locals: "arg#0": 42 (number)
-   stack:
+   stack: 
 at (anonymous) (0:17):
  - scope (global):
    -- skipped globals
@@ -104,7 +104,7 @@
    globals: "global#0": 42 (number)
  - scope (local):
    locals: "arg#0": 42 (number)
-   stack:
+   stack: 
 at (anonymous) (0:17):
  - scope (global):
    -- skipped globals
@@ -117,7 +117,7 @@
    globals: "global#0": 42 (number)
  - scope (local):
    locals: "i32_arg": 42 (number), "i32_local": 47 (number)
-   stack:
+   stack: 
 at B (liftoff) (0:76):
  - scope (global):
    globals: "global#0": 42 (number)
@@ -129,7 +129,7 @@
    globals: "global#0": 42 (number)
  - scope (local):
    locals: "arg#0": 42 (number)
-   stack:
+   stack: 
 at (anonymous) (0:17):
  - scope (global):
    -- skipped globals
diff -X /proc/self/fd/11 -Naur deps/v8/test/inspector/runtime/evaluate-repl-await-expected.txt /tmp/v8/test/inspector/runtime/evaluate-repl-await-expected.txt
--- deps/v8/test/inspector/runtime/evaluate-repl-await-expected.txt	2020-03-30 10:16:36.081788270 -0700
+++ /tmp/v8/test/inspector/runtime/evaluate-repl-await-expected.txt	2020-03-30 10:15:50.907943256 -0700
@@ -14,10 +14,10 @@
                 callFrames : [
                     [0] : {
                         columnNumber : 0
-                        functionName :
+                        functionName : 
                         lineNumber : 0
                         scriptId : <scriptId>
-                        url :
+                        url : 
                     }
                 ]
             }
@@ -50,21 +50,21 @@
                         functionName : bar
                         lineNumber : 2
                         scriptId : <scriptId>
-                        url :
+                        url : 
                     }
                     [1] : {
                         columnNumber : 6
                         functionName : foo
                         lineNumber : 6
                         scriptId : <scriptId>
-                        url :
+                        url : 
                     }
                     [2] : {
                         columnNumber : 4
-                        functionName :
+                        functionName : 
                         lineNumber : 9
                         scriptId : <scriptId>
-                        url :
+                        url : 
                     }
                 ]
             }
diff -X /proc/self/fd/11 -Naur deps/v8/test/message/fail/hashbang-incomplete-string.js /tmp/v8/test/message/fail/hashbang-incomplete-string.js
--- deps/v8/test/message/fail/hashbang-incomplete-string.js	2020-03-30 10:16:36.101789089 -0700
+++ /tmp/v8/test/message/fail/hashbang-incomplete-string.js	1969-12-31 16:00:00.000000000 -0800
@@ -1,12 +0,0 @@
-#!/usr/bin/env d8
-// Copyright 2015 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-//
-//
-
-const x = 'valid code';
-
-'incomplete string
-
-const y = 'even more valid code!';
diff -X /proc/self/fd/11 -Naur deps/v8/test/message/fail/hashbang-incomplete-string.out /tmp/v8/test/message/fail/hashbang-incomplete-string.out
--- deps/v8/test/message/fail/hashbang-incomplete-string.out	2020-03-30 10:16:36.101789089 -0700
+++ /tmp/v8/test/message/fail/hashbang-incomplete-string.out	1969-12-31 16:00:00.000000000 -0800
@@ -1,5 +0,0 @@
-*%(basename)s:10: SyntaxError: Invalid or unexpected token
-'incomplete string
-^^^^^^^^^^^^^^^^^^
-
-SyntaxError: Invalid or unexpected token
diff -X /proc/self/fd/11 -Naur deps/v8/tools/v8.xcodeproj/README.txt /tmp/v8/tools/v8.xcodeproj/README.txt
--- deps/v8/tools/v8.xcodeproj/README.txt	1969-12-31 16:00:00.000000000 -0800
+++ /tmp/v8/tools/v8.xcodeproj/README.txt	2020-03-30 10:15:51.143952862 -0700
@@ -0,0 +1,11 @@
+The Xcode project for V8 has been retired. If an Xcode project
+is needed for building on a Mac there is the option of using GYP to
+generate it. Please look in the build directory in the root of the
+V8 project. It contains the required infrastructure and a README.txt
+file explaining how to get started.
+
+Generating Xcode projects using GYP is how the Chromium
+project integrated V8 into the Mac build.
+
+The main build system for V8 is still SCons, see
+http://code.google.com/apis/v8/build.html for details.

The differences are:

  • Whitespace changes due to git node land
  • v8.xcodeproj deprecation README.txt (which we don't include because of our gitignore rules)
  • Changes from v8/v8@966bd36, which we floated before upgrading V8

So I think we're good since this PR puts us back in sync with upstream. Still not sure what happened though. Maybe it was a rebase, and not git node land?

EDIT: the space changes are causing our V8 CI to fail. I'll send a separate patch to fix that.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

@mmarchini

This comment has been minimized.

@mmarchini mmarchini closed this Apr 1, 2020
@mmarchini mmarchini reopened this Apr 1, 2020
@mmarchini
Copy link
Contributor Author

Sorry, somehow I missed the embedded string was incorrect here. I reverted the commits on master and force-pushed (within the 10 minutes grace period). Will fix the embedded string and wait a while before landing again.

This reverts commit 75da64c.

PR-URL: nodejs#32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Refs: v8/v8@8.1.307.20...8.1.307.26

PR-URL: nodejs#32521
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: nodejs#32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@mmarchini
Copy link
Contributor Author

Ok, v8 embedder string should be fixed now. I didn't bump the embedder string for the revert commit (only for the postmortem changes) since that commit shouldn't have been included in the first place (so keeping the embedder string is more coherent with the commit list).

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 2, 2020

@mmarchini

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

mmarchini added a commit that referenced this pull request Apr 4, 2020
This reverts commit 75da64c.

PR-URL: #32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mmarchini added a commit that referenced this pull request Apr 4, 2020
Refs: v8/v8@8.1.307.20...8.1.307.26

PR-URL: #32521
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
mmarchini added a commit that referenced this pull request Apr 4, 2020
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: #32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mmarchini
Copy link
Contributor Author

Landed in 8f86986...17f323e

@mmarchini mmarchini closed this Apr 4, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
This reverts commit 75da64c.

PR-URL: #32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Refs: v8/v8@8.1.307.20...8.1.307.26

PR-URL: #32521
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: #32521
Refs: v8/v8@8.1.307.20...8.1.307.26
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants