wasm: add v8 runtime. by PiotrSikora · Pull Request #66 · istio/envoy

PiotrSikora · 2019-05-09T23:41:45Z

Signed-off-by: Piotr Sikora piotrsikora@google.com

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

lambdai

Everything is good except for that I don't dare to look into the core v8.cc
Will get back to you next week.

lambdai · 2019-05-10T21:55:30Z

source/extensions/common/wasm/wasm.cc

+  } else if (wasm_vm == WasmVmNames::get().Wavm) {
+    return Wavm::createVm();
  } else {
+    UNREFERENCED_PARAMETER(wasm_vm);


Why wasm_vm is unreferenced? It is in above ifs

Good catch. This was a leftover from another change that is allowing selection of one of the VMs (or none) at the build-time.

bazel/repositories.bzl

bazel/target_recipes.bzl

lambdai · 2019-05-10T22:03:20Z

ci/build_container/build_container_ubuntu.sh

 tar -zxf cmake-3.12.3-Linux-x86_64.tar.gz -C /usr --strip-components=1
+# Install ninja 1.8.2.
+curl -sLO https://github.com/ninja-build/ninja/releases/download/v1.8.2/ninja-linux.zip
+echo "d2fea9ff33b3ef353161ed906f260d565ca55b8ca0568fa07b1d2cab90a84a07 ninja-linux.zip" | sha256sum --check


Reason? Do we need >= 1.8.2 or <=1.8.2 in this case?

ci/prebuilt/BUILD

lambdai · 2019-05-10T23:24:10Z

source/extensions/common/wasm/v8/v8.cc

+
+// Helper functions.
+
+const char* printValKind(wasm::ValKind kind) {


Hmm, I guess this is not supposed be prepared for ADL, maybe you wanna declare it static, or put in anonymous namespace

Done (declared static).

lambdai · 2019-05-10T23:24:49Z

source/extensions/common/wasm/v8/v8.cc

+  }
+}
+
+std::string printValTypes(const wasm::vec<wasm::ValType*>& types) {


Done (declared static).

lambdai · 2019-05-10T23:27:29Z

source/extensions/common/wasm/v8/v8.cc

+  return s;
+}
+
+bool equalValTypes(const wasm::vec<wasm::ValType*>& left, const wasm::vec<wasm::ValType*>& right) {


Done (declared static).

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

source/extensions/common/wasm/wasm.cc

jplevyak · 2019-05-10T02:59:14Z

test/extensions/filters/http/wasm/config_test.cc

-      absl::StrCat("vm_config:\n  vm: \"envoy.wasm.vm.wavm\"\n", "  code: { inline_bytes: \"",
-                   Base64::encode(code.data(), code.size()), "\" }\n");
+  const std::string yaml = absl::StrCat(R"EOF(
+  vm_config:


Can we turn this into a function?

To be honest, I'd rather leave it as-is.

source/extensions/common/wasm/v8/v8.cc

jplevyak · 2019-05-15T22:09:01Z

source/extensions/common/wasm/wasm.cc

-    ASSERT(std::isnan(emscripten_NaN_->get()));
-    ASSERT(std::isinf(emscripten_Infinity_->get()));
+    // TODO(PiotrSikora): re-enable those checks after v8 starts returning globals.
+    // ASSERT(std::isnan(emscripten_NaN_->get()));


Didn't you fix these?

I did & I didn't... This is something I forgot to discuss with you yesterday.

The way globals are implemented in V8, the VM retains ownership of the global instance (same as functions), but the current model passes ownership back to the abstraction layer, where it's never used (other than for those 2 asserts). Do we need to do that?

There should be a way to get the value from v8 for a global so that the Global object is just a proxy for calls into v8?

I could add a proxy, yes... My point is, that's an extra code that doesn't provide any value, since we never access globals from the host (outside of those 2 asserts).

Depending on how and whether we end up supporting threads in the context of emscripten we may need to modify the internal emscripten value stack variables. So, while we are not using the globals right now it doesn't mean we will not use them in the future. That said, we don't need the support right now and I'd rather get v8 in, so let's leave a TODO and add it to the spreadsheet and resolve this conversation :)

Added proxy, let me know if this works for you.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2019-05-16T01:24:55Z

@jplevyak @silentdai Please take another pass, I think all comments were addressed and this should be good to go. Thanks!

jplevyak · 2019-05-16T03:41:33Z

Let's get this in so I can do the merge and get the null vm in before kubecon.

Signed-off-by: Lizan Zhou <lizan@tetrate.io>

PiotrSikora added 2 commits May 9, 2019 23:11

build: update ninja to version 1.8.2.

b76eae4

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

wasm: add v8 runtime.

5f7bf10

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora requested a review from jplevyak May 9, 2019 23:41

lambdai reviewed May 10, 2019

View reviewed changes

PiotrSikora added 12 commits May 11, 2019 00:23

review: address Yuchen's comments.

a3f153a

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Merge remote-tracking branch 'istio/wasm' into PiotrSikora/wasm-v8

c98a52c

build: update wasm-c-api to latest.

2f8a050

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: disable DEBUG, since it seems to be broken.

ff8c565

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: fix typo.

4bcf3ea

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: more template magic.

3eea9ff

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: implement getMemoryOffset().

d32353f

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: remove unnecessary includes.

18e4dee

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: even more template magic.

0027504

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: even more template magic.

0bef0e7

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: verify type of each argument in module functions.

e20e042

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: add more reasons for disabled DEBUG.

51de647

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jplevyak reviewed May 15, 2019

View reviewed changes

source/extensions/common/wasm/v8/v8.cc Outdated Show resolved Hide resolved

jplevyak reviewed May 15, 2019

View reviewed changes

PiotrSikora added 4 commits May 15, 2019 23:37

v8: guard reads when parsing binary WASM module.

998a80c

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: mark callModuleFunction() args as const.

09306b8

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: re-enable NaN test, since it was fixed upstream.

20ee24d

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

v8: add proxy for globals.

36a7064

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

jplevyak approved these changes May 16, 2019

View reviewed changes

PiotrSikora merged commit dba11f3 into istio:wasm May 16, 2019

PiotrSikora deleted the wasm-v8 branch October 11, 2019 02:46

brian-avery pushed a commit that referenced this pull request Jun 30, 2020

ci: exclude config fuzz from CI (#66)

6f25c3e

Signed-off-by: Lizan Zhou <lizan@tetrate.io>


		// Helper functions.

		const char* printValKind(wasm::ValKind kind) {

Conversation

PiotrSikora commented May 9, 2019

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented May 16, 2019

Uh oh!

jplevyak commented May 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PiotrSikora May 16, 2019 •

edited

Loading