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.

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

Rust: add support for create_vm and execute #223

Closed
wants to merge 26 commits into from
Closed

Conversation

axic
Copy link
Member

@axic axic commented Mar 19, 2019

Part of #168.

@axic
Copy link
Member Author

axic commented Mar 27, 2019

Some notes:

We've got evmc_instance which must be returned by create; what the ffi really needs is that the pointer returned has the members where evmc_instance expects it, but what most VMs do is they store their private instance details after the evmc_instance in the same continuous memory area.

evmc_instance has basically one major function pointer: execute, which also receives evmc_instance as a parameter as a way to access its context

There is code do to the above, but what's missing is the user extending it with private data. I suppose that must be done with macros. The macro takes a struct type and creates the wrapper struct around it doing all the above

static [<$__vm _VERSION>]: &'static str = $__version;
}

paste::item! {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should have here a large comment explaining this will lay out memory in a format acceptable to C (repr(C)) and that's why we can include both data structures here.

Copy link
Contributor

Choose a reason for hiding this comment

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

will do

macro_rules! evmc_create_vm {
($__vm:ident, $__version:expr) => {
paste::item! {
static [<$__vm _NAME>]: &'static str = stringify!($__vm);
Copy link
Member Author

Choose a reason for hiding this comment

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

Please separate the VM name and the name of the VM struct.

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 mean the VM name may contain space and this will fail in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

discussed offline. figuring this out later

@jakelang jakelang force-pushed the rust-execute branch 2 times, most recently from 5aacc08 to 2176ca3 Compare April 23, 2019 20:59
};

let instance = unsafe { [<$__vm Instance>]::coerce_from_raw(instance) };
let result: ExecutionResult = instance.get_vm().execute(code_ref, &host);
Copy link
Member Author

Choose a reason for hiding this comment

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

Should wrap this in std::catch_panic.

@axic axic force-pushed the rust-execute branch 2 times, most recently from 2b54242 to a18ba10 Compare April 24, 2019 12:37
@axic
Copy link
Member Author

axic commented Apr 24, 2019

This is passing vmtester now with the macro monstrosity. At least locally.

@axic
Copy link
Member Author

axic commented Apr 24, 2019

@jakelang @chfast how about we split out InterfaceManager from this PR and rename it to ExecutionContext? It should also have tests on its own.

Then we can finish the core wrapper with more confidence.

@axic axic force-pushed the rust-execute branch 2 times, most recently from b4dd1a4 to db0acbf Compare April 24, 2019 18:00
@axic
Copy link
Member Author

axic commented Jun 4, 2019

Superseded by #262.

@axic axic closed this Jun 4, 2019
@axic axic deleted the rust-execute branch June 4, 2019 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants