Update wabt submodule #16

Kimundi · 2018-06-06T08:57:32Z

As a means to fix #15, I've updated the wabt submodule dependency to the current master.

Added FFI bindings for WabtWriteScriptResult
Added High-Level wrapper for WabtWriteScriptResult
Changed write_binaries and wat2wasm to only work with in-memory buffers
Removed tempdir dependency
Made ModuleBinary::into_vec() infallible

Depends on #17
Closes #9
Closes #15

pepyakin · 2018-06-06T10:06:43Z

Ah, yeah, you're right, bindings are now out of date!

I think the reason why Script::write_binaries() doesn't do anything has something to do with transition from writing directly to a file to writing into a streams (#9).

To start: write_binaries takes one wast (it's like wat format but with special scripting abilities) file and produces one json file (aka spec) with 1 or many wasm files. This json contains a specification how to run one of these files and how to check the results.

I see the following way for dealing with this:

Add a new result type WabtWriteScriptResult in sys bindings.
Add bindings for corresponding functions for this type. We will need wabt_write_script_result_release_json_output_buffer in the next step.
Update high-level wrappers. We need to introduce a special kind of result for WabtWriteScriptResult. It should hold a json buffer and buffers for all output wasm modules. We need a special methods for extracting them. Note that extracting is done via "release" method, essentially, just a move if speaking in Rust terms. You can take a look at ParseWastResult as an example.
write_binaries. Basically, we don't need to take output filename anymore to specify output filename. Instead, we can return WabtWriteScriptResult directly or return tuple consisting of json buffer and vector of wasm module buffers/Vec<u8>.
Next, we can get rid of tempfile and tempdir and all file handling altogher! Instead, we can make wat2wasm return data buffer with JSON and then parse it right away with serde! And instead of reading wasm module binaries from files we can pass vectors with modules directly!

Kimundi · 2018-06-15T18:12:30Z

Alright, so I've followed your bullet point list up to 3) so far. :) Do the higher level bindings look like you'd expect?

Kimundi · 2018-06-15T18:16:43Z

Also, a question about those take_ Functions in general - they usually pass a internal FFI raw pointer to a release function, but still call the corresponding destroy methods in their Drop impl - is that correct?

Kimundi · 2018-06-15T20:16:14Z

Alright, I'm now at 5). Modules no longer get written to temporary files, but to get rid of tempfile handling entirely I also need to make the input files live in buffers, which currently makes me trace the code paths of Script::parse.

pepyakin · 2018-06-15T21:15:46Z

Also, a question about those take_ Functions in general - they usually pass a internal FFI raw pointer to a release function, but still call the corresponding destroy methods in their Drop impl - is that correct?

Yes. release is somewhat similar to rust's Option::take: it moves the value out of the container and puts None/null instead of it.
On the other hand, destroy frees the whole structure that contains these releasable-containtainers.

Kimundi · 2018-06-15T21:20:47Z

Alright, I think this PR is now complete.

Just to be sure about the release vs destroy: In the native source its fine to call destroy after the pointer had already been released?

- Added FFI bindings for WabtWriteScriptResult - Added High-Level wrapper for WabtWriteScriptResult - Changed write_binaries and wat2wasm to only work with in-memory buffers - Removed tempdir dependency - Made ModuleBinary::into_vec() infallible

pepyakin · 2018-06-15T21:05:41Z

src/lib.rs

                self.raw_script,
                source_cstr.as_ptr(),
-                out_cstr.as_ptr(),
+                std::ptr::null(),


Could you use ptr::null?

pepyakin · 2018-06-15T21:07:31Z

src/lib.rs

+        }
+    }
+
+    fn get_module_count(&self) -> usize {


No need for superfluous get_

pepyakin · 2018-06-15T21:07:45Z

src/lib.rs

+        }
+    }
+
+    fn get_module_filename(&self, index: usize) -> &str {


No need for superfluous get_

pepyakin · 2018-06-15T21:19:39Z

src/script/mod.rs

@@ -523,19 +473,27 @@ impl<F32: FromBits<u32>, F64: FromBits<u64>> ScriptParser<F32, F64> {
            None => return Ok(None),
        };

+        let get_module = |filename, s: &Self| {
+        let mut r = None;


pepyakin · 2018-06-15T21:27:32Z

Just to be sure about the release vs. destroy: In the native source its fine to call destroy after the pointer had already been released?

That not just fine, it is required in order not to introduce a memory leak. release only gives up the ownership of data, but doesn't free it. destroy frees it : )

Kimundi · 2018-06-15T21:34:52Z

Ah, now I get it, thanks. :)

pepyakin · 2018-06-15T21:37:41Z

Ah, now I get it, thanks. :)

But now I realized one thing: these APIs seems to be not safe. The second call of take will return nullptr.

pepyakin · 2018-06-15T21:53:08Z

src/lib.rs

+                    ffi::wabt_write_script_result_release_module_output_buffer(
+                        self.raw_script_result, i)
+                };
+                let name = self.module_filename(i);


Can we instead of CStr use String::from_utf8_lossy? At least it doesn't panic upon issues with utf8

Hm, I found using the lossy variants dangerous for this, as you might generate name collisions that way, and I was under the assumption that WebAssembly embraced utf8 anyway.

I could also just store the names as CString or Vec<u8> there?

That's actually a good idea! We are only using these filenames for matching them with JSON commands, right? Even if we also need these filenames in order to display error messages, I think we can use from_utf8_lossy here (since error reporting isn't that critcial and it doesn't panic).

pepyakin · 2018-06-15T21:55:35Z

src/script/mod.rs

@@ -523,19 +473,27 @@ impl<F32: FromBits<u32>, F64: FromBits<u64>> ScriptParser<F32, F64> {
            None => return Ok(None),
        };

+        let get_module = |filename, s: &Self| {
+            let mut r = None;
+            for &(ref name, ref module) in &s.modules {


This looks like a job for a HashMap!

To clarify: I mean we could collect modules not into a Vec but into a HashMap in WabtWriteScriptResultRelease

Yeah, indeed.

pepyakin · 2018-06-16T17:20:19Z

Well done! Thank you very much!

Kimundi force-pushed the master branch from 181d7dd to aa45901 Compare June 15, 2018 21:15

Kimundi force-pushed the master branch from aa45901 to 774ee6d Compare June 15, 2018 21:21

Kimundi changed the title ~~WIP: Update wabt submodule~~ Update wabt submodule Jun 15, 2018

pepyakin reviewed Jun 15, 2018

View reviewed changes

indendation error

54a612b

Address maintainer comments

0bc7e3a

pepyakin reviewed Jun 15, 2018

View reviewed changes

pepyakin mentioned this pull request Jun 16, 2018

Fix the unsafety in the high-level bindings #18

Open

use HashMap<CString, WabtBuf>

6ceba6e

pepyakin merged commit 7a61c08 into pepyakin:master Jun 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wabt submodule #16

Update wabt submodule #16

Kimundi commented Jun 6, 2018 •

edited by pepyakin

Loading

pepyakin commented Jun 6, 2018

Kimundi commented Jun 15, 2018

Kimundi commented Jun 15, 2018

Kimundi commented Jun 15, 2018

pepyakin commented Jun 15, 2018

Kimundi commented Jun 15, 2018

pepyakin Jun 15, 2018

pepyakin Jun 15, 2018

pepyakin Jun 15, 2018

pepyakin Jun 15, 2018

pepyakin commented Jun 15, 2018

Kimundi commented Jun 15, 2018

pepyakin commented Jun 15, 2018

pepyakin Jun 15, 2018

Kimundi Jun 16, 2018

pepyakin Jun 16, 2018

pepyakin Jun 15, 2018 •

edited

Loading

Kimundi Jun 16, 2018

pepyakin commented Jun 16, 2018

Update wabt submodule #16

Update wabt submodule #16

Conversation

Kimundi commented Jun 6, 2018 • edited by pepyakin Loading

pepyakin commented Jun 6, 2018

Kimundi commented Jun 15, 2018

Kimundi commented Jun 15, 2018

Kimundi commented Jun 15, 2018

pepyakin commented Jun 15, 2018

Kimundi commented Jun 15, 2018

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

pepyakin commented Jun 15, 2018

Kimundi commented Jun 15, 2018

pepyakin commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin commented Jun 16, 2018

Kimundi commented Jun 6, 2018 •

edited by pepyakin

Loading

pepyakin Jun 15, 2018 •

edited

Loading