Stream: git-wasmtime

Topic: wasmtime / PR #4252 fix(wasmtime):`Config` methods should...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:21):

PureWhiteWu opened PR #4252 from fix/config_overwrite to main:

This commit refactored Config to use a seperate CompilerConfig field instead of operating on CompilerBuilder directly to make all its methods idempotent.
Fixes #4189

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:22):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:25):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:29):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:36):

bjorn3 created PR review comment:

These should be added to the 0.39.0 section I think.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:36):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:36):

bjorn3 created PR review comment:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:36):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:37):

bjorn3 created PR review comment:

This would need to be updated. Also where would the error occur now?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 17:37):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:04):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:04):

PureWhiteWu created PR review comment:

Thanks for your review!
The error occurs at Engine::new now.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:04):

PureWhiteWu submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin created PR review comment:

In place of the "Errors" section in the old doc comment, we should note here that flags are validated when the engine is built, and so any errors will be deferred, I think. Likewise elsewhere where an "Errors" section is removed.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin created PR review comment:

A slightly more verbose name would make this a bit clearer I think: maybe something like ensure_unset_or_given_setting? Then in the doc-comment we should say what this returns: "Returns true if successfully set or already had the given setting value, or false if the setting was explicitly set to something else previously."

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin created PR review comment:

Could we do the same sort of deferred validation for the combination of reference types and bulk memory that we do for compiler options? I'm somewhat uncomfortable with this assert existing, given that we have the right API signatures elsewhere to return the error.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:15):

cfallin created PR review comment:

Can we say a bit more here? Something about how some Wasm features require certain compiler features, and they will turn on the relevant compiler features by default, but explicitly disabling some compiler features or setting them in an incompatible way may cause errors. I think we should also note a guarantee/promise that default compiler options (i.e., if the user does not add any specific setting overrides) will always work if the underlying hardware supports what is needed.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 18:23):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton created PR review comment:

If you're up for it this explicit Clone for Config can I believe now be entirely deleted in favor of #[derive(Clone)] on the Config type. If you're feeling extra-ambitious as well the CompilerBuilder::clone method can also be removed. Either of these are fine to defer to a future PR though if you'd prefer.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton created PR review comment:

I think it's ok to remove pub(crate) on all these methods and related fields since it should be able to isolate all the usages to just this file.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton created PR review comment:

Agreed about expanding the error documentation a bit here, but I also think it would be best to keep documentation on the original config methods as well. For example Config::cranelift_flag_set could retain some documentation indicating that if an invalid setting is specified that the error will pop out during Engine::new here.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton created PR review comment:

I think that this enable-other-wasm-feature-if-this-is-enabled would be better left to Engine::new as well as otherwise this can cause confusing behavior depending on the order of the method calls.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:11):

alexcrichton created PR review comment:

If you'd like I think that this is now the only usage of the compiler_builder function so it should be fine to inline directly into this function.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:46):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu created PR review comment:

Thanks for pointing out! I think ensure_setting_unset_or_given is better, so I used that name.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu created PR review comment:

Added, except for the strategy method, because it will not return error in any situation (previously and now).

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu created PR review comment:

Can we say a bit more here? Something about how some Wasm features require certain compiler features, and they will turn on the relevant compiler features by default, but explicitly disabling some compiler features or setting them in an incompatible way may cause errors.

Fixed.

I think we should also note a guarantee/promise that default compiler options (i.e., if the user does not add any specific setting overrides) will always work if the underlying hardware supports what is needed.

I don't think it necessary to add this note explicitly, because the default options should work is intuition for users.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:50):

PureWhiteWu created PR review comment:

Fixed.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:52):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:57):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 19:58):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 09 2022 at 20:04):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2022 at 11:38):

PureWhiteWu updated PR #4252 from fix/config_overwrite to main.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2022 at 16:19):

PureWhiteWu requested alexcrichton for a review on PR #4252.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2022 at 16:19):

PureWhiteWu requested cfallin for a review on PR #4252.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2022 at 16:29):

PureWhiteWu edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2022 at 16:29):

PureWhiteWu edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2022 at 13:54):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 13 2022 at 13:54):

alexcrichton merged PR #4252.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 16:28):

thibaultcha created PR review comment:

On https://docs.wasmtime.dev/c-api/config_8h.html#ada735dd95cbee6854fc6bb27dc9591c6 it says:

If the compilation strategy selected could not be enabled then an error is returned.

I've been looking everywhere in the docs to try and find what happened to config errors, I don't think it is very clear today without looking into this PR.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 16:28):

thibaultcha submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 16:30):

thibaultcha edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 16:32):

thibaultcha edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 16:33):

thibaultcha edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:01):

PureWhiteWu submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:01):

PureWhiteWu created PR review comment:

Hi, thanks for your remind.
I changed the doc and the signature of the rust code, but I forgot the doc for c.
I've submitted #4550 to fix this.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:13):

thibaultcha submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:13):

thibaultcha created PR review comment:

@PureWhiteWu That's great! Very prompt, thank you!

A question still remains on my end but I am not sure if here's the right place for it, I'll shoot it again though:

Since wasm.h's wasm_engine_new_with_config does not return any error, where are the engine initialization errors reported when using the C API? :thinking:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:54):

PureWhiteWu submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 17:54):

PureWhiteWu created PR review comment:

Sorry, I'm not familiar with the c api, @alexcrichton do you know about this?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 19:32):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 28 2022 at 19:32):

alexcrichton created PR review comment:

You can take a look at the implementation and right now it looks like no error handling happens and if something otherwise would result in an error it would cause the process to abort.

This is probably something that would be good to fix, returning a null pointer there and perhaps adding a Wasmtime-specific API for the same operation but one that returns an error message.

Last updated: Nov 22 2024 at 17:03 UTC