Rust bindings (unsafe execution) #566

axic · 2020-10-01T13:30:00Z

TODO:

Cleanup the commits
Rustdoc
~~Fizzy error enum~~ (will do this in a new PR)

codecov · 2020-10-01T20:11:48Z

Codecov Report

Merging #566 (9d05550) into master (c290d0d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #566   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          69       69           
  Lines        9712     9712           
=======================================
  Hits         9555     9555           
  Misses        157      157

Flag	Coverage Δ
spectests	`91.47% <ø> (ø)`
unittests	`98.38% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

jakelang · 2020-10-01T21:54:36Z

bindings/rust/src/lib.rs

+
+impl Module {
+    // TODO: support imported functions{
+    pub fn instantiate(self) -> Option<Instance> {


This should probably return a Result, as failure to instantiate is more of an error condition. Also, question mark operator.

Yeah, I was wondering if I could avoid defining error types (given nothing is returned), but the question mark operator is a good point.

Need to have custom error type otherwise question mark operator won't work 😞

bindings/rust/src/lib.rs

jakelang · 2020-10-01T22:08:19Z

bindings/rust/src/lib.rs

@@ -4,8 +4,123 @@



The conventional file layout is:

<use statements> <module declarations> <trait declarations> <struct declarations> <struct methods> <trait impls>

bindings/rust/src/lib.rs

jakelang · 2020-10-01T22:16:34Z

bindings/rust/src/lib.rs

+pub type ExecutionResult = sys::FizzyExecutionResult;
+
+impl Instance {
+    pub fn execute(&mut self, func_idx: u32, args: &[Value]) -> ExecutionResult {


Ergonomics could be better if func_idx was replaced with the name. Users will probably be looking to call an export by name anyway.

The C API does not yet offer anything like that and this is a direct translation as of now.

Added function to look it up (now that the underlying API has it). Since this function is called unsafe_execute, I think it is fine to keep func_idx as it already has unsafe inputs.

Will eventually have a properly wrapped safe execute function, similar to what it is in wasmi and others.

bindings/rust/src/lib.rs

bindings/rust/Cargo.toml

bindings/rust/src/lib.rs

chfast · 2020-11-18T13:34:17Z

bindings/rust/src/lib.rs

+
+    /// The optional return value. Only a single return value is allowed in WebAssembly 1.0.
+    pub fn value(&self) -> Option<Value> {
+        if !self.0.trapped && self.0.has_value {


Documentation says otherwise, but this is too defensive. It is guaranteed that has_value if false if trapped is true.

@gumb0 suggested this. Where is it guaranteed?

Documentation says otherwise, but this is too defensive. It is guaranteed that has_value if false if trapped is true.

~~Here it's the opposite - when trapped is false, we have to check has_value.~~

What I suggested was changing if has_value -> if !trapped && has_value

Yeah I see what you mean, perhaps could be changed back, with a change to C API documentation and some new asserts in tests.

What I suggested was changing if has_value -> if !trapped && has_value

This is what the code has now.

The ExecuteResult constructor guarantees it. If you want to keep it, swap the checks: has_value && !trapped.

I can also add the trapped as an assertion, which is only present in debug builds:

pub fn value(&self) -> Option<Value> { if self.0.has_value { assert!(!self.0.trapped); Some(self.0.value) } else { None } }

The ExecuteResult constructor guarantees it.

The C API has no constructor guarantee for it though?

The ExecuteResult constructor guarantees it.

The C API has no constructor guarantee for it though?

No constructor guarantees but FizzyExecutionResult wrap(const fizzy::ExecutionResult&) just copies all three fields always.

I can also add the trapped as an assertion, which is only present in debug builds:

This variant looks good.

I'm ok with changing trapped check to assertion.

bindings/rust/src/lib.rs

gumb0 · 2020-11-19T15:53:48Z

bindings/rust/src/lib.rs

+
+    /// The optional return value. Only a single return value is allowed in WebAssembly 1.0.
+    pub fn value(&self) -> Option<Value> {
+        if !self.0.trapped && self.0.has_value {


I'm ok with changing trapped check to assertion.

axic · 2020-11-19T16:30:00Z

@chfast @gumb0 made the assertion change

axic force-pushed the rust branch from 5cb04fd to 09e03d7 Compare October 1, 2020 13:30

axic marked this pull request as draft October 1, 2020 13:30

axic force-pushed the capi_execute branch from c517410 to 815497a Compare October 1, 2020 13:47

axic force-pushed the rust branch 10 times, most recently from dc15f97 to 49ef6bb Compare October 1, 2020 19:50

jakelang reviewed Oct 1, 2020

View reviewed changes

axic force-pushed the rust branch from b10b70a to 4713366 Compare October 1, 2020 23:01

gumb0 force-pushed the capi_execute branch 2 times, most recently from dfdbfbf to 6525b9d Compare October 5, 2020 11:17

axic force-pushed the rust branch from 7a5ce71 to c016ff9 Compare October 6, 2020 19:59

axic commented Oct 7, 2020

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

gumb0 force-pushed the capi_execute branch from 6525b9d to 2a3d5c7 Compare October 8, 2020 16:13

axic force-pushed the rust branch from 2aae50d to 84aa8b5 Compare October 8, 2020 20:15

axic changed the base branch from capi_execute to capi-execute-dynamic-module October 12, 2020 13:10

gumb0 force-pushed the capi-execute-dynamic-module branch 2 times, most recently from f38f7e5 to 59419bd Compare October 12, 2020 16:34

Base automatically changed from capi-execute-dynamic-module to master October 12, 2020 17:16

axic force-pushed the rust branch 2 times, most recently from 54e4c4e to d2e6a1f Compare October 12, 2020 17:21

axic marked this pull request as ready for review October 12, 2020 17:23

chfast reviewed Nov 9, 2020

View reviewed changes

bindings/rust/Cargo.toml Show resolved Hide resolved

gumb0 reviewed Nov 9, 2020

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

gumb0 reviewed Nov 9, 2020

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

gumb0 reviewed Nov 9, 2020

View reviewed changes

bindings/rust/src/lib.rs Outdated Show resolved Hide resolved

axic force-pushed the rust branch from a4502be to 0b65193 Compare November 17, 2020 21:54

axic changed the title ~~Rust bindings (execution)~~ Rust bindings (unsafe execution) Nov 18, 2020

axic changed the base branch from master to capi-find-exports November 18, 2020 12:59

axic force-pushed the rust branch from 0b65193 to 79428df Compare November 18, 2020 13:00

Base automatically changed from capi-find-exports to master November 18, 2020 13:04

chfast reviewed Nov 18, 2020

View reviewed changes

bindings/rust/src/lib.rs Show resolved Hide resolved

bindings/rust/src/lib.rs Show resolved Hide resolved

chfast reviewed Nov 18, 2020

View reviewed changes

axic force-pushed the rust branch 4 times, most recently from e45448f to 4eb205c Compare November 18, 2020 19:11

gumb0 approved these changes Nov 19, 2020

View reviewed changes

axic added 5 commits November 19, 2020 16:28

rust: add Module and parse

659dcdc

rust: add Instance and instantiate

07ec11d

rust: add execute and Value/ExecutionResult

0ae940e

rust: add From trait and as_*() helpers on Value

5f2df00

rust: implement find_exported_function

9d05550

axic force-pushed the rust branch from 4eb205c to 9d05550 Compare November 19, 2020 16:29

axic requested review from chfast and gumb0 November 19, 2020 16:30

chfast approved these changes Nov 19, 2020

View reviewed changes

axic merged commit bc60be5 into master Nov 19, 2020

axic deleted the rust branch November 19, 2020 19:20

axic mentioned this pull request Feb 4, 2021

Rust binding #722

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust bindings (unsafe execution) #566

Rust bindings (unsafe execution) #566

axic commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

jakelang Oct 1, 2020

axic Oct 1, 2020

axic Oct 7, 2020

jakelang Oct 1, 2020

jakelang Oct 1, 2020

axic Oct 1, 2020

axic Nov 6, 2020 •

edited

Loading

chfast Nov 18, 2020

axic Nov 18, 2020

gumb0 Nov 18, 2020 •

edited

Loading

axic Nov 18, 2020

chfast Nov 18, 2020

axic Nov 18, 2020 •

edited

Loading

axic Nov 18, 2020

gumb0 Nov 19, 2020

chfast Nov 19, 2020

gumb0 Nov 19, 2020

gumb0 Nov 19, 2020

axic commented Nov 19, 2020

Rust bindings (unsafe execution) #566

Rust bindings (unsafe execution) #566

Conversation

axic commented Oct 1, 2020 • edited Loading

codecov bot commented Oct 1, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Nov 19, 2020

axic commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

axic Nov 6, 2020 •

edited

Loading

gumb0 Nov 18, 2020 •

edited

Loading

axic Nov 18, 2020 •

edited

Loading