Skip to content
This repository was archived by the owner on Apr 8, 2026. It is now read-only.

Move evmc_host_interface out of evmc_host_context#427

Merged
chfast merged 6 commits into
masterfrom
host_interface
Nov 5, 2019
Merged

Move evmc_host_interface out of evmc_host_context#427
chfast merged 6 commits into
masterfrom
host_interface

Conversation

@chfast
Copy link
Copy Markdown
Member

@chfast chfast commented Sep 20, 2019

On the C API level (and also ABI) the host_interface (think: an object vtable) and host_context (think: pointer to an object's data) are separated. This actually makes creating language bindings easier - no need for workarounds as in https://github.com/ethereum/evmc/blob/master/bindings/go/evmc/evmc.go#L26-L30.

Language bindings can still hide this by providing single OOP interface for Host (both for Client and VM side).

@chfast
Copy link
Copy Markdown
Member Author

chfast commented Sep 25, 2019

@axic Do you want me to finish Rust part here?

@chfast chfast force-pushed the host_interface branch 3 times, most recently from 076cb83 to 72da957 Compare October 31, 2019 09:46
@chfast
Copy link
Copy Markdown
Member Author

chfast commented Nov 4, 2019

@axic @jakelang can you finish Rust changes here, please?

@axic
Copy link
Copy Markdown
Member

axic commented Nov 4, 2019

@chfast fixed compilation, but haven't reviewed the changes yet.

@axic axic force-pushed the host_interface branch 4 times, most recently from 7dab19e to 3f865ba Compare November 4, 2019 19:59
Comment thread bindings/rust/evmc-vm/src/container.rs Outdated
emit_log: None,
};
let mut host_context = ::evmc_sys::evmc_host_context { _unused: [] };
let mut host_context = ::evmc_sys::evmc_host_context::default();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is it possible to pass a null pointer instead of creating host_context object?

Copy link
Copy Markdown
Member

@axic axic Nov 4, 2019

Choose a reason for hiding this comment

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

Yeah, that should be an option too. Will make Rust cry though, a mutable nullpointer reference lol.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just tried it and it feels like to be simple leaving it as is, for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried it too, but feels like null pointers is Rust's advanced feature.

@chfast
Copy link
Copy Markdown
Member Author

chfast commented Nov 4, 2019

Ok, thanks a lot. I will clean up git history and put it for review.

@chfast chfast requested review from axic, gumb0 and jakelang and removed request for jakelang November 4, 2019 20:50
@chfast chfast marked this pull request as ready for review November 4, 2019 20:51
Comment thread include/evmc/evmc.h Outdated
Comment thread include/evmc/evmc.h
Comment thread bindings/rust/evmc-declare/src/lib.rs Outdated

// TODO: context is optional in case of the "precompiles" capability
if instance.is_null() || context.is_null() || msg.is_null() || (code.is_null() && code_size != 0) {
if instance.is_null() || host.is_null() || context.is_null() || msg.is_null() || (code.is_null() && code_size != 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Depending on #427 (comment) we may need to remove context.is_null() here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The context may always be null. VM never needs to dereference it.
Now the TODO comment should be applied to the host as in the case of "precompiles" it is not useful and may be null — this can be discussed further in #350.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed the context part. Lets leave host for #350.

@axic
Copy link
Copy Markdown
Member

axic commented Nov 5, 2019

@chfast simplified it to deal with null pointers. Also allows context to be null now.

@chfast chfast merged commit a15e493 into master Nov 5, 2019
@chfast chfast deleted the host_interface branch November 5, 2019 11:53
@axic axic mentioned this pull request Dec 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants