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

wasm: move implementation details into WasmVm abstraction layer. #47

Merged
merged 4 commits into from
Mar 22, 2019

Conversation

PiotrSikora
Copy link

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

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora requested a review from jplevyak March 16, 2019 03:32
@istio-testing
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@PiotrSikora
Copy link
Author

@jplevyak in the interest of cleaning up the interface, I pulled getFunction() into the abstraction layer. It doesn't leak any VM implementation details, which I think is beneficial. The only downside is that there are no templates for virtual methods in C++, so the interface gets a bit more noisy, but it seems worth it. What do you think?

(I'll migrate registerCallback() and makeGlobal() as well, but I didn't want to spend too much time on this, if you disagree with the direction of this change)

cc @silentdai

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora marked this pull request as ready for review March 18, 2019 23:41
@PiotrSikora PiotrSikora changed the title [WIP] wasm: pull getFunction() into WasmVm abstraction layer. wasm: pull implementation details WasmVm abstraction layer. Mar 18, 2019
@PiotrSikora PiotrSikora changed the title wasm: pull implementation details WasmVm abstraction layer. wasm: move implementation details into WasmVm abstraction layer. Mar 18, 2019
Copy link

@jplevyak jplevyak left a comment

Choose a reason for hiding this comment

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

I am not opposed to this, but it does add some noise. If you like it better, then feel free to merge. If you can wrap up a bit of larger repetitions in macros and you think it is cleaner I would appreciate it. I find that if the macro is defined directly above the repeated use it improves readability and cuts down on toil.

@@ -221,6 +232,71 @@ struct Wavm : public WasmVm {

void getInstantiatedGlobals();

void getFunction(absl::string_view functionName, WasmCall0Void* f) override {

Choose a reason for hiding this comment

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

Perhaps we could wrap this repetition up in a macro?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Author

@jplevyak I agree that it adds a bit noise. Ideally, we'd be able to get/register any function passed as argument, but that doesn't seem to be possible. I definitely prefer that those methods are part of the runtime implementation, and not leaked into the abstraction layer, though... If you have any better idea on how to achieve that, I'm all ears.

@PiotrSikora PiotrSikora merged commit 0e61789 into istio:wasm Mar 22, 2019
rlenglet pushed a commit that referenced this pull request Aug 13, 2019
* Add Shared queues and test for both shared data and shared queues.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
brian-avery added a commit that referenced this pull request Sep 29, 2020
* Patch 1

* Patch 3

* Patch 4

* Remove manual
brian-avery added a commit that referenced this pull request Sep 29, 2020
istio-testing pushed a commit that referenced this pull request Oct 2, 2020
* [1.7] Fix CVE-2020-25017 (#47)

* Patch 1

* Patch 3

* Patch 4

* Remove manual

* Revert "[1.7] Fix CVE-2020-25017 (#47)" (#48)

This reverts commit c336863.

* [1.7] Fix CVE-2020-25017 (#49)

* Patch 1

* Patch 3

* Patch 4

* Remove manual

* Add fix from 1.6
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.

3 participants