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.

Sign up for GitHub

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

Jump to bottom

rust: implement safer execution wrapper #652

Merged
merged 2 commits into from
Jan 29, 2021
Merged

rust: implement safer execution wrapper #652

merged 2 commits into from
Jan 29, 2021

Conversation

axic
Copy link
Member

@axic axic commented Nov 18, 2020

No description provided.

@codecov
Copy link

codecov bot commented Nov 18, 2020 •
edited
Loading

Codecov Report

Merging #652 (5e5e43f) into master (c820769) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #652   +/-   ##
=======================================
  Coverage   99.31%   99.31%           
=======================================
  Files          72       72           
  Lines       10628    10628           
=======================================
  Hits        10555    10555           
  Misses         73       73
Flag Coverage Δ
spectests 90.84% <ø> (ø)
unittests 99.31% <ø> (ø)

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

Base automatically changed from rust to master November 19, 2020 19:20
@axic axic force-pushed the rust-safe-execute branch from 1151a2a to ccac5fc Compare November 19, 2020 19:24
chfast
chfast approved these changes Dec 10, 2020
View reviewed changes
bindings/rust/src/lib.rs Outdated Show resolved Hide resolved
bindings/rust/src/lib.rs Outdated
@@ -160,6 +160,11 @@ impl From<ExecutionResult> for sys::FizzyExecutionResult {
}

impl Instance {
/// Get a read-only pointer to the module.
unsafe fn get_instance_module(&self) -> *const sys::FizzyModule {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
unsafe fn get_instance_module(&self) -> *const sys::FizzyModule {
unsafe fn get_module(&self) -> *const sys::FizzyModule {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I tried to mirror the underlying C API names, but probably it makes no sense.

gumb0
gumb0 reviewed Dec 14, 2020
View reviewed changes
bindings/rust/src/lib.rs Outdated
// TODO: validate input types too

let ret = unsafe { self.unsafe_execute(func_idx, args, depth) };
// Fail if output is expected, but none was returned.
Copy link
Collaborator

@gumb0 gumb0 Dec 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It shouldn't happen unless there're bugs in execution, so maybe this should be an assertion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The opposite could also be asserted: function type is void, but something is returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should use assertions when there is an API bug, and errors when it is a user validation question. I guess in this case assertion is fine as we get the func_idx from the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should do it?

        if !ret.trapped()
        {
           assert!((func_type.output == sys::FizzyValueTypeVoid) == !ret.value().is_some());
        }

@axic axic force-pushed the rust-safe-execute branch 2 times, most recently from a84f843 to 7f24a40 Compare January 28, 2021 12:35
axic
axic commented Jan 28, 2021
View reviewed changes
bindings/rust/src/lib.rs
depth: i32,
) -> Result<ExecutionResult, ()> {
let func_idx = self.find_exported_function_index(&name);
if func_idx.is_none() {
Copy link
Member Author

Choose a reason for hiding this comment

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

If get #677 merged, these could be replaced with let func_idx = self.find_exported_function_index(&name)?; where the question mark operator means errors are bubbled up.

I would however go with this PR as is first, and then merge the error one showing the improvement.

@axic axic requested review from gumb0 and chfast January 28, 2021 12:38
@axic axic marked this pull request as ready for review January 28, 2021 12:38
axic
axic commented Jan 28, 2021
View reviewed changes
bindings/rust/build.rs
@@ -29,6 +29,8 @@ fn main() {
// https://github.com/rust-lang-nursery/rust-bindgen/issues/947#issuecomment-327100002
.layout_tests(false)
.whitelist_function("fizzy_.*")
.whitelist_var("Fizzy.*")
Copy link
Member Author

Choose a reason for hiding this comment

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

Needed for FizzyValueTypeVoid.

axic
axic commented Jan 28, 2021
View reviewed changes
bindings/rust/src/lib.rs
if func_type.inputs_size != args.len() {
return Err(());
}
// TODO: validate input types too
Copy link
Member Author

@axic axic Jan 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

For type validation need to revamp the type of Value (turning it into an enum data type with conversions) here to include type information. Maybe better separately as it is quite large.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #705.

gumb0
gumb0 approved these changes Jan 28, 2021
View reviewed changes
Copy link
Collaborator

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Looks good, for completeness you could add a test with a function having some params and calling it with 0 or incorrect number of arguments.

@axic
Copy link
Member Author

axic commented Jan 28, 2021

Looks good, for completeness you could add a test with a function having some params and calling it with 0 or incorrect number of arguments.

The last line with foo does the "more args than required" case. Will add a "less args than required" case too.

@axic axic force-pushed the rust-safe-execute branch from 6c67475 to bd46cf2 Compare January 28, 2021 18:59
@axic axic requested a review from gumb0 January 28, 2021 19:00
@axic axic force-pushed the rust-safe-execute branch from bd46cf2 to 8fe90d3 Compare January 28, 2021 19:38
@axic axic mentioned this pull request Jan 28, 2021
Merged
@axic axic force-pushed the rust-safe-execute branch 3 times, most recently from 448c9a9 to 693ad0d Compare January 29, 2021 11:48
@axic axic force-pushed the rust-safe-execute branch from 693ad0d to 5e5e43f Compare January 29, 2021 11:57
@axic axic merged commit 3c34262 into master Jan 29, 2021
@axic axic deleted the rust-safe-execute branch January 29, 2021 12:32
@axic axic mentioned this pull request Feb 4, 2021
Open
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gumb0 gumb0 gumb0 approved these changes

@chfast chfast Awaiting requested review from chfast

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@axic @chfast @gumb0