Enable some warnings and turn on Control Flow Guard (#9)

- reenable some V8 disabled warnings;
- fix some warnings in adapter code;
- turn on CFG (Control Flow Guard);
- add waitForDebugger flag and use default debugger port;

config.json

@@ -1,4 +1,4 @@
 {
-    "version": "0.2.4",
+    "version": "0.2.5",
     "v8ref": "refs/branch-heads/8.1"
 }

scripts/build.ps1

@@ -10,37 +10,6 @@ param(
 $workpath = Join-Path $SourcesPath "build"
 $jsigitpath = Join-Path $SourcesPath "src"
 
-$gn_snippet = @'
-
-group("jsi") {
-  deps = [
-    "jsi:v8jsi",
-  ]
-}
-
-'@
-
-if (!(Select-String -Path (Join-Path $workpath "v8build\v8\BUILD.gn") -Pattern 'jsi:v8jsi' -Quiet)) {
-    Add-Content -Path (Join-Path $workpath "v8build\v8\BUILD.gn") $gn_snippet
-}
-
-#TODO (#2): This is ugly, but it's the least intrusive way to override this config across all targets
-$FixNeededPath = Join-Path $workpath "v8build\v8\build\config\win\BUILD.gn"
-(Get-Content $FixNeededPath) -replace (":static_crt", ":dynamic_crt") | Set-Content $FixNeededPath
-
-#TODO: This is temporary until Office moves to the new toolset with FH4 support (ETA: May 2020)
-(Get-Content $FixNeededPath) -replace ('/Zc:sizedDealloc-', @'
-/Zc:sizedDealloc-","-d2FH4-
-'@) | Set-Content $FixNeededPath
-
-(Get-Content $FixNeededPath) -replace ('ldflags = \[\]', @'
-if (is_clang) {
-    ldflags = []
-} else {
-    ldflags = ["-d2:-FH4-"]
-}
-'@) | Set-Content $FixNeededPath
-
 Remove-Item (Join-Path $workpath "v8build\v8\jsi") -Recurse -ErrorAction Ignore
 Copy-Item $jsigitpath -Destination (Join-Path $workpath "v8build\v8\jsi") -Recurse -Force
 

scripts/fetch_code.ps1

@@ -31,15 +31,15 @@ $config = Get-Content (Join-Path $SourcesPath "config.json") | Out-String | Conv
 $CheckOutVersion = (git checkout FETCH_HEAD) | Out-String
 & gclient sync
 
-#TODO (#2): Submit PR upstream to Google for this fix
-$FixNeededPath = Join-Path $workpath "v8build\v8\src\base\template-utils.h"
-(Get-Content $FixNeededPath) -replace ("#include <utility>", "#include <utility>`n#include <functional>") | Set-Content $FixNeededPath
+# Apply patches
+& git apply (Join-Path $SourcesPath "scripts\patch\src.diff")
 
-if (!$PSVersionTable.Platform -or $IsWindows) {
-    #TODO (#2): once the fix for the upstream hack lands, we can remove this patch (see https://bugs.chromium.org/p/chromium/issues/detail?id=1033106)
-    Copy-Item -Path ( Join-Path $SourcesPath "scripts\patch\tool_wrapper.py") -Destination (Join-Path $workpath "v8build\v8\build\toolchain\win\tool_wrapper.py") -Force
-}
+Push-Location (Join-Path $workpath "v8build\v8\build")
+& git apply (Join-Path $SourcesPath "scripts\patch\build.diff")
 
+# To re-generate these patches in the future use something like: 'git diff --output=path\src.diff --ignore-cr-at-eol' in the respective folders
+
+Pop-Location
 Pop-Location
 Pop-Location
 

scripts/patch/build.diff

@@ -0,0 +1,70 @@
+diff --git a/config/compiler/BUILD.gn b/config/compiler/BUILD.gn
+index d602b4494..29ee27ba2 100644
+--- a/config/compiler/BUILD.gn
++++ b/config/compiler/BUILD.gn
+@@ -1360,8 +1360,8 @@ config("default_warnings") {
+       "/wd4512",  # Assignment operator could not be generated.
+       "/wd4610",  # Class can never be instantiated, constructor required.
+       "/wd4838",  # Narrowing conversion. Doesn't seem to be very useful.
+-      "/wd4995",  # 'X': name was marked as #pragma deprecated
+-      "/wd4996",  # Deprecated function warning.
++      #"/wd4995",  # 'X': name was marked as #pragma deprecated
++      #"/wd4996",  # Deprecated function warning.
+ 
+       # These are variable shadowing warnings that are new in VS2015. We
+       # should work through these at some point -- they may be removed from
+@@ -1658,7 +1658,7 @@ config("no_chromium_code") {
+       "/W3",  # Warning level 3.
+       "/wd4800",  # Disable warning when forcing value to bool.
+       "/wd4267",  # TODO(jschuh): size_t to int.
+-      "/wd4996",  # Deprecated function warning.
++      #"/wd4996",  # Deprecated function warning.
+     ]
+     defines += [
+       "_CRT_NONSTDC_NO_WARNINGS",
+diff --git a/config/win/BUILD.gn b/config/win/BUILD.gn
+index 37a710e30..a657e1e91 100644
+--- a/config/win/BUILD.gn
++++ b/config/win/BUILD.gn
+@@ -75,7 +75,7 @@ config("compiler") {
+ 
+   cflags += [
+     # Work around crbug.com/526851, bug in VS 2015 RTM compiler.
+-    "/Zc:sizedDealloc-",
++    "/Zc:sizedDealloc-","-d2FH4-",
+   ]
+ 
+   if (is_clang) {
+@@ -136,7 +136,11 @@ config("compiler") {
+     cflags += [ "/Brepro" ]
+   }
+ 
+-  ldflags = []
++  if (is_clang) {
++    ldflags = []
++} else {
++    ldflags = ["-d2:-FH4-"]
++}
+ 
+   if (use_lld) {
+     # lld defaults to writing the current time in the pe/coff header.
+@@ -462,7 +466,7 @@ config("default_crt") {
+       configs = [ ":dynamic_crt" ]
+     } else {
+       # Desktop Windows: static CRT.
+-      configs = [ ":static_crt" ]
++      configs = [ ":dynamic_crt" ]
+     }
+   }
+ }
+@@ -616,3 +620,10 @@ config("cygprofile_instrumentation") {
+     "-finstrument-functions-after-inlining",
+   ]
+ }
++# Control Flow Guard (CFG)
++config("win_msvc_cfg") {
++  if (!is_clang && !is_debug) {
++    cflags = [ "/guard:cf", "/Qspectre" ]
++    ldflags = [ "/guard:cf" ]
++  }
++}

scripts/patch/src.diff

@@ -0,0 +1,42 @@
+diff --git a/BUILD.gn b/BUILD.gn
+index 0ffa2b794d..e808e4bd79 100644
+--- a/BUILD.gn
++++ b/BUILD.gn
+@@ -4678,3 +4678,10 @@ if (!build_with_chromium && v8_use_perfetto) {
+     }
+   }  # host_toolchain
+ }  # if (!build_with_chromium && v8_use_perfetto)
++
++group("jsi") {
++  deps = [
++    "jsi:v8jsi",
++  ]
++}
++
+diff --git a/include/v8.h b/include/v8.h
+index d6f60c2562..e64ce7b803 100644
+--- a/include/v8.h
++++ b/include/v8.h
+@@ -9024,6 +9024,11 @@ class V8_EXPORT Isolate {
+    */
+   MicrotasksPolicy GetMicrotasksPolicy() const;
+ 
++#ifdef _MSC_VER
++#pragma warning(push)
++#pragma warning(disable: 4996)
++#endif
++
+   /**
+    * Adds a callback to notify the host application after
+    * microtasks were run on the default MicrotaskQueue. The callback is
+@@ -9050,6 +9055,10 @@ class V8_EXPORT Isolate {
+   void RemoveMicrotasksCompletedCallback(
+       MicrotasksCompletedCallbackWithData callback, void* data = nullptr);
+ 
++#ifdef _MSC_VER
++#pragma warning(pop)
++#endif
++
+   /**
+    * Sets a callback for counting the number of times a feature of V8 is used.
+    */

scripts/patch/tool_wrapper.py

@@ -1,195 +0,0 @@
-# Copyright (c) 2012 The Chromium Authors. All rights reserved.
-# Use of this source code is governed by a BSD-style license that can be
-# found in the LICENSE file.
-
-"""Utility functions for Windows builds.
-
-This file is copied to the build directory as part of toolchain setup and
-is used to set up calls to tools used by the build that need wrappers.
-"""
-
-from __future__ import print_function
-
-import os
-import re
-import shutil
-import subprocess
-import stat
-import sys
-
-def superflush(pe_name):
-  return None
-
-BASE_DIR = os.path.dirname(os.path.abspath(__file__))
-
-# A regex matching an argument corresponding to the output filename passed to
-# link.exe.
-_LINK_EXE_OUT_ARG = re.compile('/OUT:(?P<out>.+)$', re.IGNORECASE)
-
-def main(args):
-  exit_code = WinTool().Dispatch(args)
-  if exit_code is not None:
-    sys.exit(exit_code)
-
-
-class WinTool(object):
-  """This class performs all the Windows tooling steps. The methods can either
-  be executed directly, or dispatched from an argument list."""
-
-  def _UseSeparateMspdbsrv(self, env, args):
-    """Allows to use a unique instance of mspdbsrv.exe per linker instead of a
-    shared one."""
-    if len(args) < 1:
-      raise Exception("Not enough arguments")
-
-    if args[0] != 'link.exe':
-      return
-
-    # Use the output filename passed to the linker to generate an endpoint name
-    # for mspdbsrv.exe.
-    endpoint_name = None
-    for arg in args:
-      m = _LINK_EXE_OUT_ARG.match(arg)
-      if m:
-        endpoint_name = re.sub(r'\W+', '',
-            '%s_%d' % (m.group('out'), os.getpid()))
-        break
-
-    if endpoint_name is None:
-      return
-
-    # Adds the appropriate environment variable. This will be read by link.exe
-    # to know which instance of mspdbsrv.exe it should connect to (if it's
-    # not set then the default endpoint is used).
-    env['_MSPDBSRV_ENDPOINT_'] = endpoint_name
-
-  def Dispatch(self, args):
-    """Dispatches a string command to a method."""
-    if len(args) < 1:
-      raise Exception("Not enough arguments")
-
-    method = "Exec%s" % self._CommandifyName(args[0])
-    return getattr(self, method)(*args[1:])
-
-  def _CommandifyName(self, name_string):
-    """Transforms a tool name like recursive-mirror to RecursiveMirror."""
-    return name_string.title().replace('-', '')
-
-  def _GetEnv(self, arch):
-    """Gets the saved environment from a file for a given architecture."""
-    # The environment is saved as an "environment block" (see CreateProcess
-    # and msvs_emulation for details). We convert to a dict here.
-    # Drop last 2 NULs, one for list terminator, one for trailing vs. separator.
-    pairs = open(arch).read()[:-2].split('\0')
-    kvs = [item.split('=', 1) for item in pairs]
-    return dict(kvs)
-
-  def ExecDeleteFile(self, path):
-    """Simple file delete command."""
-    if os.path.exists(path):
-      os.unlink(path)
-
-  def ExecRecursiveMirror(self, source, dest):
-    """Emulation of rm -rf out && cp -af in out."""
-    if os.path.exists(dest):
-      if os.path.isdir(dest):
-        def _on_error(fn, path, dummy_excinfo):
-          # The operation failed, possibly because the file is set to
-          # read-only. If that's why, make it writable and try the op again.
-          if not os.access(path, os.W_OK):
-            os.chmod(path, stat.S_IWRITE)
-          fn(path)
-        shutil.rmtree(dest, onerror=_on_error)
-      else:
-        if not os.access(dest, os.W_OK):
-          # Attempt to make the file writable before deleting it.
-          os.chmod(dest, stat.S_IWRITE)
-        os.unlink(dest)
-
-    if os.path.isdir(source):
-      shutil.copytree(source, dest)
-    else:
-      shutil.copy2(source, dest)
-      # Try to diagnose crbug.com/741603
-      if not os.path.exists(dest):
-        raise Exception("Copying of %s to %s failed" % (source, dest))
-
-  def ExecLinkWrapper(self, arch, use_separate_mspdbsrv, *args):
-    """Filter diagnostic output from link that looks like:
-    '   Creating library ui.dll.lib and object ui.dll.exp'
-    This happens when there are exports from the dll or exe.
-    """
-    env = self._GetEnv(arch)
-    if use_separate_mspdbsrv == 'True':
-      self._UseSeparateMspdbsrv(env, args)
-    if sys.platform == 'win32':
-      args = list(args)  # *args is a tuple by default, which is read-only.
-      args[0] = args[0].replace('/', '\\')
-    # https://docs.python.org/2/library/subprocess.html:
-    # "On Unix with shell=True [...] if args is a sequence, the first item
-    # specifies the command string, and any additional items will be treated as
-    # additional arguments to the shell itself.  That is to say, Popen does the
-    # equivalent of:
-    #   Popen(['/bin/sh', '-c', args[0], args[1], ...])"
-    # For that reason, since going through the shell doesn't seem necessary on
-    # non-Windows don't do that there.
-    pe_name = None
-    for arg in args:
-      m = _LINK_EXE_OUT_ARG.match(arg)
-      if m:
-        pe_name = m.group('out')
-    link = subprocess.Popen(args, shell=sys.platform == 'win32', env=env,
-                            stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-    # Read output one line at a time as it shows up to avoid OOM failures when
-    # GBs of output is produced.
-    for line in link.stdout:
-      if (not line.startswith('   Creating library ') and
-          not line.startswith('Generating code') and
-          not line.startswith('Finished generating code')):
-        print(line)
-    result = link.wait()
-    if result == 0 and sys.platform == 'win32':
-      superflush(pe_name)
-    return result
-
-  def ExecAsmWrapper(self, arch, *args):
-    """Filter logo banner from invocations of asm.exe."""
-    env = self._GetEnv(arch)
-    if sys.platform == 'win32':
-      # Windows ARM64 uses clang-cl as assembler which has '/' as path
-      # separator, convert it to '\\' when running on Windows.
-      args = list(args) # *args is a tuple by default, which is read-only
-      args[0] = args[0].replace('/', '\\')
-    popen = subprocess.Popen(args, shell=True, env=env,
-                             stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
-    out, _ = popen.communicate()
-    for line in out.splitlines():
-      if not line.startswith(' Assembling: '):
-        print(line)
-    return popen.returncode
-
-  def ExecRcWrapper(self, arch, *args):
-    """Converts .rc files to .res files."""
-    env = self._GetEnv(arch)
-    args = list(args)
-    rcpy_args = args[:]
-    rcpy_args[0:1] = [sys.executable, os.path.join(BASE_DIR, 'rc', 'rc.py')]
-    rcpy_args.append('/showIncludes')
-    return subprocess.call(rcpy_args, env=env)
-
-  def ExecActionWrapper(self, arch, rspfile, *dirname):
-    """Runs an action command line from a response file using the environment
-    for |arch|. If |dirname| is supplied, use that as the working directory."""
-    env = self._GetEnv(arch)
-    # TODO(scottmg): This is a temporary hack to get some specific variables
-    # through to actions that are set after GN-time. http://crbug.com/333738.
-    for k, v in os.environ.items():
-      if k not in env:
-        env[k] = v
-    args = open(rspfile).read()
-    dirname = dirname[0] if dirname else None
-    return subprocess.call(args, shell=True, env=env, cwd=dirname)
-
-
-if __name__ == '__main__':
-  sys.exit(main(sys.argv[1:]))

src/BUILD.gn

@@ -44,6 +44,10 @@ target("shared_library", "v8jsi") {
   configs += [ "//:internal_config_base", "//build/config/compiler:exceptions" ]
   configs -= [ "//build/config/compiler:no_exceptions" ]
 
+  if (is_win) {
+    configs += [ "//build/config/win:win_msvc_cfg" ]
+  }
+
   deps = [
     "//:v8_headers",
     "//:v8_monolith",

src/V8JsiRuntime.cpp

@@ -561,7 +561,10 @@ V8Runtime::V8Runtime(V8RuntimeArgs &&args) : args_(std::move(args)) {
         "JSIRuntime context",
         args_.inspectorPort);
     inspector_agent_->start();
-    inspector_agent_->waitForDebugger();
+
+    if (args_.waitForDebugger) {
+      inspector_agent_->waitForDebugger();
+    }
   }
 #endif
 

src/V8Platform.cpp

@@ -44,6 +44,8 @@ uint64_t ETWTracingController::AddTraceEvent(
   }
 
 #if defined(_WIN32) && !defined(__clang__)
+#pragma warning(push)
+#pragma warning(disable: 4002) // too many arguments for function-like macro invocation
   EventWriteGENERIC_EVENT(
       phase,
       name,
@@ -78,6 +80,7 @@ uint64_t ETWTracingController::AddTraceEvent(
       names[8],
       types[8],
       values[8]);
+#pragma warning(pop)
 #endif
 
   return 0;
@@ -108,6 +111,8 @@ uint64_t ETWTracingController::AddTraceEventWithTimestamp(
   }
 
 #if defined(_WIN32) && !defined(__clang__)
+#pragma warning(push)
+#pragma warning(disable: 4002) // too many arguments for function-like macro invocation
   EventWriteGENERIC_EVENT(
       phase,
       name,
@@ -142,6 +147,7 @@ uint64_t ETWTracingController::AddTraceEventWithTimestamp(
       names[8],
       types[8],
       values[8]);
+#pragma warning(pop)
 #endif
 
   return 0;

src/inspector/inspector_agent.cpp

@@ -372,13 +372,13 @@ void InspectorConsoleCall(const v8::FunctionCallbackInfo<v8::Value> &info) {
         config_object->Set(context, in_call_key, v8::True(isolate)).FromJust());
     CHECK(
         !inspector_method.As<v8::Function>()
-             ->Call(context, info.Holder(), call_args.size(), call_args.data())
+             ->Call(context, info.Holder(), static_cast<int>(call_args.size()), call_args.data())
              .IsEmpty());
   }
 
   v8::TryCatch try_catch(info.GetIsolate());
   static_cast<void>(node_method.As<v8::Function>()->Call(
-      context, info.Holder(), call_args.size(), call_args.data()));
+      context, info.Holder(), static_cast<int>(call_args.size()), call_args.data()));
   CHECK(config_object->Delete(context, in_call_key).FromJust());
   if (try_catch.HasCaught())
     try_catch.ReThrow();
@@ -470,7 +470,7 @@ std::unique_ptr<v8_inspector::StringBuffer> ToProtocolString(
     return v8_inspector::StringBuffer::create(v8_inspector::StringView());
   }
   v8::Local<v8::String> string_value = v8::Local<v8::String>::Cast(value);
-  size_t len = string_value->Length();
+  int len = string_value->Length();
   std::basic_string<uint16_t> buffer(len, '\0');
   string_value->Write(v8::Isolate::GetCurrent(), &buffer[0], 0, len);
   return v8_inspector::StringBuffer::create(

src/inspector/inspector_socket.cpp

@@ -249,7 +249,7 @@ static std::vector<char> encode_frame_hybi17(const std::vector<char>& message) {
 
 static ws_decode_result decode_frame_hybi17(const std::vector<char>& buffer,
                                             bool client_frame,
-                                            int* bytes_consumed,
+                                            size_t* bytes_consumed,
                                             std::vector<char>* output,
                                             bool* compressed) {
   *bytes_consumed = 0;
@@ -353,7 +353,7 @@ class WsHandler : public ProtocolHandler {
 
   void OnData(std::vector<char>* data) override {
     // 1. Parse.
-    int processed = 0;
+    size_t processed = 0;
     do {
       processed = ParseWsFrames(*data);
       // 2. Fix the data size & length
@@ -403,8 +403,8 @@ class WsHandler : public ProtocolHandler {
     SendClose();
   }
 
-  int ParseWsFrames(const std::vector<char>& buffer) {
-    int bytes_consumed = 0;
+  size_t ParseWsFrames(const std::vector<char>& buffer) {
+    size_t bytes_consumed = 0;
     std::vector<char> output;
     bool compressed = false;
 

src/inspector/inspector_tcp.cpp

@@ -64,7 +64,7 @@ void tcp_connection::read_loop_async() {
       std::vector<char> vc;
       //const char* start = boost::asio::buffer_cast<const char*>(input_buffer_.data());
       vc.reserve(bytes_transferred);
-      for (int c = 0; c < bytes_transferred; c++) {
+      for (size_t c = 0; c < bytes_transferred; c++) {
         vc.push_back(buffer_.data()[c]);
       }
 

src/inspector/inspector_utils.cpp

@@ -199,7 +199,7 @@ size_t base64_encode(const char* src, size_t slen, char* dst, size_t dlen) {
 
   i = 0;
   k = 0;
-  n = slen / 3 * 3;
+  n = static_cast<int>(slen) / 3 * 3;
 
   while (i < n) {
     a = src[i + 0] & 0xff;

src/public/V8JsiRuntime.h

@@ -85,9 +85,10 @@ struct V8RuntimeArgs {
   bool enableGCTracing{false};
 
   bool enableInspector{false};
+  bool waitForDebugger{false};
 
   // chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=localhost:8888
-  uint16_t inspectorPort{8888};
+  uint16_t inspectorPort{9229};
 
   size_t initial_heap_size_in_bytes{0};
   size_t maximum_heap_size_in_bytes{0};