proxy-wasm-cpp-host icon indicating copy to clipboard operation
proxy-wasm-cpp-host copied to clipboard

build: update v8 to 12.7.224.18

Open mpwarres opened this issue 1 year ago • 12 comments

  • Update v8.patch
  • Remove v8_include.patch which is no longer needed
  • Remove dependency on chromium_base_trace_event_common

This PR is stacked on top of #411.

mpwarres avatar Aug 15 '24 22:08 mpwarres

ERROR: /home/runner/.cache/bazel/_bazel_runner/ae21fb6b2e26a9267f16b46bb90ea321/external/v8/bazel/defs.bzl:472:22: Use of Starlark transition without allowlist attribute '_allowlist_function_transition'. See Starlark transitions documentation for details and usage: @v8//:bazel/defs.bzl NORMAL

Looks like I need to add a patch to v8's bazel/defs.bzl to do this. IIUC this requirement is no longer present in the most recent version of Bazel, but maybe that's not used yet in our CI.

mpwarres avatar Aug 15 '24 22:08 mpwarres

ERROR: /home/runner/.cache/bazel/_bazel_runner/ae21fb6b2e26a9267f16b46bb90ea321/external/v8/bazel/defs.bzl:472:22: Use of Starlark transition without allowlist attribute '_allowlist_function_transition'. See Starlark transitions documentation for details and usage: @v8//:bazel/defs.bzl NORMAL

Looks like I need to add a patch to v8's bazel/defs.bzl to do this. IIUC this requirement is no longer present in the most recent version of Bazel, but maybe that's not used yet in our CI.

They removed it quite recently (in V8 v12.4, see: https://github.com/v8/v8/commit/b26554ec368e9553782012c96aa5e99b163eaff2), so you should be able to simply revert that commit.

PiotrSikora avatar Aug 16 '24 09:08 PiotrSikora

They removed it quite recently (in V8 v12.4, see: v8/v8@b26554e), so you should be able to simply revert that commit.

Thanks for the pointer; updated patch to revert that commit. Let's see how this fares in CI.

mpwarres avatar Aug 16 '24 15:08 mpwarres

Looks like it needs abseil.

PiotrSikora avatar Aug 16 '24 16:08 PiotrSikora

Clang builds on Linux with engine=v8 actually succeed for me locally now, even though they fail in CI. One difference is that CI is using llvm-14 whereas my local build is using llvm-16.

Unfortunately, changing to use -std=c++20 (which v8 now requires as of v8/v8@f06f6d1a45fe87f2669268275b19df6c23d6a815) breaks compilation with gcc. And building with -std=c++17 plus a patch to v8 that reverts v8/v8@f06f6d1a45fe87f2669268275b19df6c23d6a815 still encounters other build issues.

mpwarres avatar Aug 17 '24 01:08 mpwarres

It would be possible to update our runners to use clang 16 instead of clang 14 by explicitly installing it in the runner as described in https://github.com/actions/runner-images/issues/8659#issuecomment-1780570741. In fact, workerd, which also builds v8 with Bazel, does this. However, Envoy still builds with clang-14 (CI documentation), so I think we would be setting the Envoy build up for problems if we depend on a different version of Clang.

I'm back to trying to build with clang-14 + -std=c++17 for other Wasm runtimes, and -std=c++20 for v8 (as added) by their build rules), but am currently running into https://github.com/actions/runner-images/issues/8659. The recommended fix for that issue is to use a newer version of clang, which as mentioned above isn't a great option here. So I am trying to see if I can kludge around it with yet another patch.

mpwarres avatar Aug 17 '24 21:08 mpwarres

I'm back to trying to build with clang-14 + -std=c++17 for other Wasm runtimes, and -std=c++20 for v8 (as added) by their build rules), but am currently running into actions/runner-images#8659. The recommended fix for that issue is to use a newer version of clang, which as mentioned above isn't a great option here. So I am trying to see if I can kludge around it with yet another patch.

My comment above was incorrect. actions/runner-images#8659 was fixed in https://github.com/actions/runner-images/issues/9679; the build errors I was seeing were due to my local build environment.

So there is now a choice WRT C++ standard versions. proxy-wasm-cpp-host currently builds with -std=c++17], while std=c++20 is used by both v8 and Envoy. Which raises the following choice:

A. Continue to build proxy-wasm-cpp-host with C++17, letting v8 build rules add C++20 for v8 targets. This will require std=c++20 to be specified for proxy-wasm-cpp-host's :v8_lib target, though, since otherwise, building it trips across v8's requirement of C++20.

B. Continue to build proxy-wasm-cpp-host with C++17, and patch v8 to revert https://github.com/v8/v8/commit/f06f6d1a45fe87f2669268275b19df6c23d6a815 which added the C++20 requirement.

C. Switch to C++20 and resolve the build errors it currently raises (see failed CI for #411).

I'm inclined to go with (C) since it matches both Envoy and v8 build settings more closely. I will pursue that further in #411 and stack this PR on top of that.

mpwarres avatar Aug 18 '24 22:08 mpwarres

Rebased on top of #411

mpwarres avatar Aug 19 '24 18:08 mpwarres

"Linux/x86_64 with GCC" failure:

external/v8/src/execution/isolate.h: In static member function 'static uint32_t v8::internal::Isolate::c_entry_fp_offset()':
external/v8/src/execution/isolate.h:879:44: error: 'offsetof' within non-standard-layout type 'v8::internal::Isolate' is conditionally-supported [-Werror=invalid-offsetof]
  879 |     return static_cast<uint32_t>(OFFSET_OF(Isolate, isolate_data_) +
external/v8/src/execution/isolate.h:879:34: note: in expansion of macro 'OFFSET_OF'
  879 |     return static_cast<uint32_t>(OFFSET_OF(Isolate, isolate_data_) +
      |                                  ^~~~~~~~~

This looks to be the same issue as envoyproxy/envoy#21674 and https://crbug.com/42204065 . Since I don't want to change the version of gcc being used, looks like I need to add -Wno-invalid-offsetof.

mpwarres avatar Aug 19 '24 18:08 mpwarres

The next compilation failure, v8-specific:

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
In file included from external/v8/src/compiler/turboshaft/store-store-elimination-phase.cc:5:
In file included from external/v8/src/compiler/turboshaft/store-store-elimination-phase.h:8:
In file included from external/v8/src/compiler/turboshaft/phase.h:13:
In file included from external/v8/src/codegen/optimized-compilation-info.h:12:
In file included from external/v8/src/codegen/source-position-table.h:14:
In file included from external/v8/src/zone/zone-containers.h:25:
external/v8/src/base/small-vector.h:25:3: error: static_assert failed due to requirement '::v8::base::is_trivially_copyable<std::pair<const v8::internal::compiler::turboshaft::PhiOp *, const v8::internal::compiler::turboshaft::OpIndex>>::value' "T should be trivially copyable"
  ASSERT_TRIVIALLY_COPYABLE(T);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/v8/src/base/macros.h:215:3: note: expanded from macro 'ASSERT_TRIVIALLY_COPYABLE'
  static_assert(::v8::base::is_trivially_copyable<T>::value, \
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/v8/src/compiler/turboshaft/loop-unrolling-reducer.h:564:65: note: in instantiation of template class 'v8::base::SmallVector<std::pair<const v8::internal::compiler::turboshaft::PhiOp *, const v8::internal::compiler::turboshaft::OpIndex>, 16>' requested here
  base::SmallVector<std::pair<const PhiOp*, const OpIndex>, 16> phis;
                                                                ^
1 error generated.

Looks like I need to cherrypick https://github.com/v8/v8/commit/35888fee7bbaaaf1f02ccc88a95c9a336fc790bc .

mpwarres avatar Aug 19 '24 19:08 mpwarres

On to the next gcc compilation error:

external/v8/src/ast/scopes.cc: In lambda function:
external/v8/src/ast/scopes.cc:2594:25: error: implicit capture of 'this' via '[=]' is deprecated in C++20 [-Werror=deprecated]
 2594 |                         [=](Variable* var) { return !MustAllocate(var); });
      |                         ^
external/v8/src/ast/scopes.cc:2594:25: note: add explicit 'this' or '*this' capture
At global scope:
cc1plus: note: unrecognized command-line option '-Wno-deprecated-this-capture' may have been intended to silence earlier diagnostics

Seems gcc wants -Wno-deprecated.

mpwarres avatar Aug 19 '24 21:08 mpwarres

Ok, maybe -Wno-deprecated wasn't it:

Run bazel test --verbose_failures --test_output=errors --define engine=v8 --config=gcc -- //test/... 
   bazel test --verbose_failures --test_output=errors --define engine=v8 --config=gcc -- //test/... 
  shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
INFO: Found applicable config definition build:gcc in file /home/runner/work/proxy-wasm-cpp-host/proxy-wasm-cpp-host/.bazelrc: --action_env=BAZEL_COMPILER=gcc --action_env=CC=gcc --action_env=CXX=g++ --cxxopt -Wno-invalid-offsetof -Wno-deprecated
ERROR: -Wno-deprecated :: Invalid options syntax: -Wno-deprecated
Error: Process completed with exit code 2.

mpwarres avatar Aug 19 '24 21:08 mpwarres

Obsoleted by #440

mpwarres avatar Jul 15 '25 12:07 mpwarres