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

src: remove uses of deprecated wasm TransferrableModule #30026

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 18, 2019

(Blocked on #30020)

WasmModuleObject::TransferrableModule is deprecated and will be removed
in V8 v8.0. Replace all uses by CompiledWasmModule.

Refs: v8#101

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 18, 2019
@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Oct 18, 2019
@targos
Copy link
Member

targos commented Oct 18, 2019

../src/node_messaging.cc: In member function ‘virtual v8::MaybeLocal<v8::WasmModuleObject> node::worker::{anonymous}::DeserializerDelegate::GetWasmModuleFromId(v8::Isolate*, uint32_t)’:
../src/node_messaging.cc:86:12: error: ‘FromCompiledModule’ is not a member of ‘v8::WasmModuleObject’
     return WasmModuleObject::FromCompiledModule(
            ^~~~~~~~~~~~~~~~

@addaleax
Copy link
Member Author

@targos Yeah, I’ve just edited the description to reflect that this needs to land after V8 7.9 :)

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Nov 8, 2019
WasmModuleObject::TransferrableModule is deprecated and will be removed
in V8 v8.0. Replace all uses by CompiledWasmModule.

Refs: v8#101
@addaleax addaleax force-pushed the messaging-wasm-transferrable2compiled branch from 2288b08 to c3440a8 Compare November 8, 2019 15:10
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Nov 8, 2019
WasmModuleObject::TransferrableModule is deprecated and will be removed
in V8 v8.0. Replace all uses by CompiledWasmModule.

Refs: v8#101

PR-URL: #30026
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Carlier <[email protected]>
@addaleax
Copy link
Member Author

addaleax commented Nov 8, 2019

Landed in 69f19f4

@addaleax addaleax closed this Nov 8, 2019
@addaleax addaleax deleted the messaging-wasm-transferrable2compiled branch November 8, 2019 22:25
@MylesBorins
Copy link
Contributor

@addaleax WasmModuleObject::FromCompiledModule doesn't exist in V8 7.8. Should we keep this open for when the backport happens or not bother backporting to 13.x?

@BridgeAR
Copy link
Member

This lands cleanly after applying V8 7.9 but it does not compile. I guess we have to wait with this until V8 8.0?

../src/node_messaging.cc: In member function ‘virtual v8::MaybeLocal<v8::WasmModuleObject> node::worker::{anonymous}::DeserializerDelegate::GetWasmModuleFromId(v8::Isolate*, uint32_t)’:
../src/node_messaging.cc:86:30: error: ‘FromCompiledModule’ is not a member of ‘v8::WasmModuleObject’
   86 |     return WasmModuleObject::FromCompiledModule(
      |                              ^~~~~~~~~~~~~~~~~~
In file included from ../src/util.h:27,
                 from ../src/aliased_buffer.h:7,
                 from ../src/env.h:27,
                 from ../src/node_messaging.h:6,
                 from ../src/node_messaging.cc:1:
../deps/v8/include/v8.h: In instantiation of ‘void v8::PersistentBase<T>::SetWeak(P*, typename v8::WeakCallbackInfo<P>::Callback, v8::WeakCallbackType) [with P = node::BaseObject; T = v8::Object; typename v8::WeakCallbackInfo<P>::Callback = void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)]’:
../src/base_object-inl.h:123:42:   required from here
../deps/v8/include/v8.h:10400:16: warning: cast between incompatible function types from ‘v8::WeakCallbackInfo<node::BaseObject>::Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<node::BaseObject>&)’} to ‘Callback’ {aka ‘void (*)(const v8::WeakCallbackInfo<void>&)’} [-Wcast-function-type]
10400 |                reinterpret_cast<Callback>(callback), type);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@addaleax
Copy link
Member Author

@BridgeAR This likely won’t work on v13.x the way we are doing the 7.9 backport, but it also doesn’t really have to land on v13.x anyway. I’ll add the dont-land label.

codebytere added a commit to electron/electron that referenced this pull request Sep 1, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 2, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 4, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 14, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.