rust: support memory manipulation #609

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.

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

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 gumb0 and chfast 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust: support memory manipulation #609

rust: support memory manipulation #609

axic commented Oct 15, 2020

codecov bot commented Oct 15, 2020 •

edited

Loading

chfast left a comment

axic commented Dec 29, 2020 •

edited

Loading

chfast commented Feb 1, 2021

axic commented Feb 4, 2021

gumb0 commented Feb 9, 2021

axic commented Feb 9, 2021 •

edited

Loading

gumb0 Feb 10, 2021

axic Feb 11, 2021

axic Feb 11, 2021

chfast left a comment

	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 {

rust: support memory manipulation #609

rust: support memory manipulation #609

Conversation

axic commented Oct 15, 2020

codecov bot commented Oct 15, 2020 • edited Loading

Codecov Report

chfast left a comment

Choose a reason for hiding this comment

axic commented Dec 29, 2020 • edited Loading

chfast commented Feb 1, 2021

axic commented Feb 4, 2021

gumb0 commented Feb 9, 2021

axic commented Feb 9, 2021 • edited Loading

gumb0 Feb 10, 2021

Choose a reason for hiding this comment

axic Feb 11, 2021

Choose a reason for hiding this comment

axic Feb 11, 2021

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2020 •

edited

Loading

axic commented Dec 29, 2020 •

edited

Loading

axic commented Feb 9, 2021 •

edited

Loading