rust: support memory manipulation by axic · Pull Request #609 · wasmx/fizzy

axic · 2020-10-15T15:40:12Z

No description provided.

codecov · 2020-10-15T16:28:04Z

Codecov Report

Merging #609 (4684eb1) into master (c37b101) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #609      +/-   ##
==========================================
+ Coverage   99.32%   99.35%   +0.03%     
==========================================
  Files          72       73       +1     
  Lines       10691    11388     +697     
==========================================
+ Hits        10619    11315     +696     
- Misses         72       73       +1

Flag	Coverage Δ
rust	`99.85% <100.00%> (?)`
spectests	`90.84% <ø> (ø)`
unittests	`99.32% <ø> (ø)`

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

Impacted Files	Coverage Δ
bindings/rust/src/lib.rs	`99.85% <100.00%> (ø)`

bindings/rust/src/lib.rs

chfast

Copying memory back and forth does not look like good idea. Rust should some kind of mutable slice allowing free access to wasm memory (read/write).

bindings/rust/src/lib.rs

axic · 2020-12-29T22:12:05Z

Copying memory back and forth does not look like good idea. Rust should some kind of mutable slice allowing free access to wasm memory (read/write).

~~This PR does mutable slicing.~~

Right. We can definitely have a separate function exposing the underlying memory as an unsafe API, but this is the safe API. It is modelled after other Rust VMs.

chfast · 2021-02-01T08:55:01Z

Right. We can definitely have a separate function exposing the underlying memory as an unsafe API, but this is the safe API. It is modelled after other Rust VMs.

Why exposing the underlying memory is unsafe?

axic · 2021-02-04T14:37:58Z

Right. We can definitely have a separate function exposing the underlying memory as an unsafe API, but this is the safe API. It is modelled after other Rust VMs.

Why exposing the underlying memory is unsafe?

Exposing a slice obtained via std::slice::from_raw_parts (i.e. the underlying C++ allocated memory) is considered unsafe as ownership is not managed by Rust.

gumb0 · 2021-02-08T14:49:38Z

Tests for memory_get / memory_set with empty slices are needed.

bindings/rust/src/lib.rs

gumb0 · 2021-02-09T19:26:44Z

Looks good, but still not sure what's best to allow for slice ranges #609 (comment)

axic · 2021-02-09T20:40:33Z

Squashed and added #609 (comment) as a second commit. We can decide this tomorrow.

gumb0 · 2021-02-10T11:06:19Z

bindings/rust/src/lib.rs

+        let offset = offset as usize;
+        let memory_size = self.memory_size();
+        // Empty slices are allowed, but ensure both starting and ending offsets are valid.
+        if memory_size == 0 || offset > memory_size || (offset + size) > memory_size {


offset > memory_size condition is redundant then, when it is true (offset + size) > memory_size will always be true.

Suggested change

if memory_size == 0 || offset > memory_size || (offset + size) > memory_size {

if memory_size == 0 || (offset + size) > memory_size {

Also for the case of (memory 0) it would be perhaps logical to allow (0, 0) slice, then the condition should make an error only the absence of memory

Suggested change

if memory_size == 0 || offset > memory_size || (offset + size) > memory_size {

if !sys::fizzy_module_has_memory(self.get_module()) || (offset + size) > memory_size {

offset > memory_size condition is redundant then, when it is true (offset + size) > memory_size will always be true.

Was a bad attempt for overflow check. Decided to just use offset.checked_add(size).is_none() for overflow check.

bindings/rust/src/lib.rs

axic · 2021-02-11T19:35:36Z

bindings/rust/src/lib.rs

+    ) -> Result<core::ops::Range<usize>, ()> {
+        // This should be safe given usize::BITS >= u32::BITS, see https://doc.rust-lang.org/std/primitive.usize.html.
+        let offset = offset as usize;
+        let has_memory = unsafe { sys::fizzy_module_has_memory(self.get_module()) };


Actually with solution in #728, I would cache this in the Instance { has_memory: bool }.

chfast

Can be for now, but I see some design issues with it.

The fizzy_get_instance_memory_size is called twice.
You almost always wants memory size and pointer in the same time. C should return both in single call.

bindings/rust/src/lib.rs

Introduce unsafe API to acquire memory slices and a safe API to set/get memory.

axic force-pushed the rust branch 4 times, most recently from e1b3750 to 3934809 Compare October 27, 2020 15:57

axic force-pushed the rust branch 4 times, most recently from 1377814 to ca8ecb2 Compare November 6, 2020 23:07

axic commented Nov 6, 2020

View reviewed changes

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

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

axic force-pushed the rust-memory branch from 081832a to 441bdfb Compare November 18, 2020 20:03

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

Base automatically changed from rust to master November 19, 2020 19:20

axic force-pushed the rust-memory branch from 441bdfb to 2ae7622 Compare November 19, 2020 19:24

chfast reviewed Dec 10, 2020

View reviewed changes

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

axic force-pushed the rust-memory branch 2 times, most recently from 1a2d6d3 to ae1fa64 Compare January 29, 2021 13:07

axic force-pushed the rust-memory branch 2 times, most recently from f9e523c to e0952b3 Compare February 4, 2021 10:47

axic mentioned this pull request Feb 4, 2021

Rust binding #722

Open

16 tasks

axic force-pushed the rust-memory branch from e0952b3 to c89d7cf Compare February 5, 2021 19:46

gumb0 reviewed Feb 8, 2021

View reviewed changes

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

gumb0 reviewed Feb 8, 2021

View reviewed changes

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

axic requested a review from gumb0 February 9, 2021 11:24

gumb0 reviewed Feb 9, 2021

View reviewed changes

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

axic requested a review from gumb0 February 9, 2021 18:39

axic force-pushed the rust-memory branch from 8fa7a59 to ad0022f Compare February 9, 2021 18:39

gumb0 reviewed Feb 9, 2021

View reviewed changes

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

gumb0 approved these changes Feb 9, 2021

View reviewed changes

axic force-pushed the rust-memory branch from ad0022f to c301265 Compare February 9, 2021 20:39

axic requested a review from gumb0 February 9, 2021 20:40

gumb0 reviewed Feb 10, 2021

View reviewed changes

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

axic force-pushed the rust-memory branch from c301265 to 1cfdc2c Compare February 11, 2021 19:34

axic requested a review from gumb0 February 11, 2021 19:34

axic commented Feb 11, 2021

View reviewed changes

gumb0 approved these changes Feb 12, 2021

View reviewed changes

chfast approved these changes Feb 12, 2021

View reviewed changes

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

axic force-pushed the rust-memory branch from 1cfdc2c to d00fa8a Compare February 12, 2021 14:07

chfast reviewed Feb 12, 2021

View reviewed changes

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

axic requested review from chfast and gumb0 February 12, 2021 15:21

chfast approved these changes Feb 12, 2021

View reviewed changes

rust: support memory

4684eb1

Introduce unsafe API to acquire memory slices and a safe API to set/get memory.

axic force-pushed the rust-memory branch from 4fb20b8 to 4684eb1 Compare February 12, 2021 16:19

gumb0 approved these changes Feb 12, 2021

View reviewed changes

axic merged commit a58c3df into master Feb 12, 2021

axic deleted the rust-memory branch February 12, 2021 16:58

	if memory_size == 0 \|\| offset > memory_size \|\| (offset + size) > memory_size {
	if memory_size == 0 \|\| (offset + size) > memory_size {

	if memory_size == 0 \|\| offset > memory_size \|\| (offset + size) > memory_size {
	if !sys::fizzy_module_has_memory(self.get_module()) \|\| (offset + size) > memory_size {

Conversation

axic commented Oct 15, 2020

Uh oh!

codecov bot commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

chfast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axic commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chfast commented Feb 1, 2021

Uh oh!

axic commented Feb 4, 2021

Uh oh!

gumb0 commented Feb 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gumb0 commented Feb 9, 2021

Uh oh!

axic commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gumb0 Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

axic Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

axic Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

chfast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 15, 2020 •

edited

Loading

axic commented Dec 29, 2020 •

edited

Loading

axic commented Feb 9, 2021 •

edited

Loading