Add support for WASM intrinsic globals. (STACKED on wasm-module, see commit messages) #36
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| if (is_emscripten_) { | ||
| wasm_vm_->makeModule("global"); | ||
| emscripten_NaN_ = makeGlobal(wasm_vm_.get(), "global", "NaN", std::nan("0")); | ||
| emscripten_Infinity_ = |
There was a problem hiding this comment.
Is there a list of name in global module?
There was a problem hiding this comment.
Unfortunately the Emscripten ABI isn't stable. The best source for that information is the Emscripten source code for the various versions.
| void Wasm::establishEnvironment() { | ||
| if (is_emscripten_) { | ||
| wasm_vm_->makeModule("global"); | ||
| emscripten_NaN_ = makeGlobal(wasm_vm_.get(), "global", "NaN", std::nan("0")); |
There was a problem hiding this comment.
"global", "NaN" and "Infinity" are core concept. Could you use const variable and aggregate them in single place?
| template <typename R, typename... Args> | ||
| void getFunctionWavm(WasmVm* vm, absl::string_view functionName, | ||
| std::function<R(Context*, Args...)>*); | ||
|
|
||
| template <typename T> | ||
| std::unique_ptr<Global<T>> makeGlobalWavm(WasmVm* vm, absl::string_view moduleName, |
There was a problem hiding this comment.
A naive question
Where is makeGlobalWavm instantiated?
There was a problem hiding this comment.
It is in the wasm/wavm/wavm.cc file. It is encapsulated in that file. The idea is to make the details of the individual VM implementations private. I choose that because wavm/wavm.h is included in wasm/wasm.cc but not wasm/wasm.h to prevent circularizing the includes. I am planning on moving class WavmVm into a new file and reorganizing the includes here which would enable moving this declaration into the wavm/ directory, after clearing the current PR backlog.
| @@ -245,6 +245,12 @@ struct AsyncClientHandler : public Http::AsyncClient::Callbacks { | |||
| Http::AsyncClient::Request* request; | |||
| }; | |||
|
|
|||
| template <typename T> struct Global { | |||
There was a problem hiding this comment.
Another naive question
Where can I find a inherited Global ?
There was a problem hiding this comment.
in wavm/wavm.cc. That is the private VM specific implementation of the Global class.
There was a problem hiding this comment.
Thanks! I didn't find it because github doesn't render large diff.
| @@ -245,6 +245,12 @@ struct AsyncClientHandler : public Http::AsyncClient::Callbacks { | |||
| Http::AsyncClient::Request* request; | |||
| }; | |||
|
|
|||
| template <typename T> struct Global { | |||
There was a problem hiding this comment.
Thanks! I didn't find it because github doesn't render large diff.
| : Intrinsics::GenericGlobal<T>(module, name.c_str(), value), wavm_(wavm) {} | ||
| virtual ~WavmGlobal() {} | ||
|
|
||
| T get(); |
| virtual ~WavmGlobal() {} | ||
|
|
||
| T get(); | ||
| void set(const T& t); |
There was a problem hiding this comment.
virtual void set(const T& t) override;
BTW: is multiple line comment supported? I didn't find an approach to mark multiple lines that need attention.
| } | ||
| WAVM::Runtime::LinkResult linkResult = linkModule(irModule_, rootResolver); | ||
| moduleInstance_ = instantiateModule(compartment_, module_, std::move(linkResult.resolvedImports), | ||
| std::string(name)); | ||
| memory_ = getDefaultMemory(moduleInstance_); | ||
| for (auto& p : intrinsicGlobals_) { |
There was a problem hiding this comment.
Feel free to ignore: This green area is quite isolated from the existing logic. It doesn't use any stack variable so it's reasonable to name this piece some other day.
| @@ -585,6 +620,36 @@ template void getFunctionWavm<uint32_t, uint32_t, uint32_t, uint32_t, uint32_t, | |||
| std::function<uint32_t(Context*, uint32_t, uint32_t, uint32_t, uint32_t, uint32_t, uint32_t, | |||
| uint32_t, uint32_t, uint32_t, uint32_t)>*); | |||
|
|
|||
| template <typename T> T getValue(IR::Value) {} | |||
There was a problem hiding this comment.
Yes, but it will fail to compile if is actually used because it doesn't return anything.
| template <> double getValue(IR::Value v) { return v.f64; } | ||
|
|
||
| template <typename T> T WavmGlobal<T>::get() { | ||
| return getValue<T>(getGlobalValue(wavm_->context_, global_)); |
There was a problem hiding this comment.
This implementation remind me that the return type should be std::decay_t to avoid get reference (e.g, T = int32_t&)
There was a problem hiding this comment.
discussed offline. This is only used for templates explicitly instantiated in this file, so there is no chance of accidental &.
| std::unique_ptr<Global<T>> makeGlobalWavm(WasmVm* vm, absl::string_view moduleName, | ||
| absl::string_view name, T initialValue) { | ||
| auto wavm = static_cast<Common::Wasm::Wavm::Wavm*>(vm); | ||
| auto g = |
There was a problem hiding this comment.
Why are you doing this? You may always write as below. There is no penalty and it is exception safe.
auto g_ptr = std::make_unique<WavmGlobal<T>>(...);
wavm->intrinsicGlobals_[...] = g_ptr.get();
return g_ptr;
There was a problem hiding this comment.
It is a bit more complicated because the return type is std::unique_ptr<Global>> which the compiler will not auto-coerce from 'g'. Instead I have to do a std::static_pointer_cast<> and this works around that by using the automatic std::unique_ptr constructor coercion which does upcast "correctly" (IMO std::unique_ptr should do that was well, maybe in a later version of C++).
I'll change it.
There was a problem hiding this comment.
My bad. You are right. The problem only happens with shared_ptr. Done.
| EXPECT_FALSE(code.empty()); | ||
| auto context = std::make_unique<TestContext>(wasm.get()); | ||
| EXPECT_CALL(*context, scriptLog(spdlog::level::info, Eq("NaN nan"))); | ||
| EXPECT_CALL(*context, scriptLog(spdlog::level::warn, Eq("inf inf"))); |
Signed-off-by: John Plevyak <jplevyak@gmail.com>
This patch separates the Resource class from the resource manager implementation and allows for resource limit tracking in other parts of the code base. Signed-off-by: Tony Allen <tony@allen.gg> Signed-off-by: Lizan Zhou <lizan@tetrate.io> Co-authored-by: Lizan Zhou <lizan@tetrate.io>
Add support for WASM intrinsic globals.