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

build: update v8 to 12.7.224.18 #409

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

mpwarres
Copy link
Contributor

@mpwarres mpwarres commented Aug 15, 2024

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

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

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: v8/v8@b26554e), so you should be able to simply revert that commit.

@mpwarres
Copy link
Contributor Author

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

Looks like it needs abseil.

@mpwarres
Copy link
Contributor Author

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@f06f6d1) breaks compilation with gcc. And building with -std=c++17 plus a patch to v8 that reverts v8/v8@f06f6d1 still encounters other build issues.

@mpwarres
Copy link
Contributor Author

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 actions/runner-images#8659 (comment). 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 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.

Signed-off-by: Michael Warres <[email protected]>
@mpwarres
Copy link
Contributor Author

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 actions/runner-images#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 v8/v8@f06f6d1 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.

Signed-off-by: Michael Warres <[email protected]>
- Update v8.patch
- Remove v8_include.patch which is no longer needed
- Remove dependency on chromium_base_trace_event_common

Signed-off-by: Michael Warres <[email protected]>
…63eaff2

This re-adds the use of _allowlist_function_transition, which is needed by
Bazel 6.5.0.

Signed-off-by: Michael Warres <[email protected]>
@mpwarres
Copy link
Contributor Author

Rebased on top of #411

This isn't strictly necessary in this PR for CI to pass, however I encountered
a need for it in child PR proxy-wasm#409, and seemed more appropriate to add here.

Signed-off-by: Michael Warres <[email protected]>
@mpwarres
Copy link
Contributor Author

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

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 v8/v8@35888fe .

@mpwarres
Copy link
Contributor Author

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

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.

Comment on lines +68 to +69
build:gcc --cxxopt -Wno-invalid-offsetof --host_cxxopt -Wno-invalid-offsetof
build:gcc --cxxopt -Wno-deprecated --host_cxxopt -Wno-deprecated
Copy link
Member

Choose a reason for hiding this comment

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

As a general rule, those V8 workarounds should be:

  1. added to v8.patch as patches against V8's bazel/defs.bzl,
  2. upstreamed to V8, so we don't need to maintain those local workarounds forever.

Especially, because those cxxopts won't be propagated to dependent projects (i.e. Envoy).

flags: --config=clang-asan-strict --define=crypto=system
flags: --config=clang-asan --define=crypto=system
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants