proxy-wasm / proxy-wasm-cpp-host

WebAssembly for Proxies (C++ host implementation)
Apache License 2.0
84 stars 69 forks source link

build: update v8 to 12.7.224.18 #409

Open mpwarres opened 3 months ago

mpwarres commented 3 months ago

This PR is stacked on top of #411.

mpwarres commented 3 months ago
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.

PiotrSikora commented 3 months ago
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.

mpwarres commented 3 months ago

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.

PiotrSikora commented 3 months ago

Looks like it needs abseil.

mpwarres commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Rebased on top of #411

mpwarres commented 3 months ago

"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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.