wasmtime / PR #9778 Wasmtime: support a notion of "custom... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9778 Wasmtime: support a notion of "custom...

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:15):

cfallin requested fitzgen for a review on PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:15):

cfallin requested wasmtime-core-reviewers for a review on PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:15):

cfallin opened PR #9778 from cfallin:custom-code-publisher to bytecodealliance:main:

In some no_std environments, virtual memory usage is generally prohibited for performance-predictability reasons, but the MMU hardware is still in use for permissions (e.g., W^X write-xor-execute). Occasional changes to page mapping permissions are thus necessary when new modules are loaded dynamically, and are acceptable in that context. Wasmtime needs a way to support "publishing" code (making it executable) in such environments.

Rather than try to segment the signals-based-traps divide further, and piece out the code-publishing parts from the heap parts, and backdoor a path to mprotect in an otherwise no_std build, in this PR I have opted to add a trait an impl of which the embedder can provide to the Config to implement custom actions for "code publish". This otherwise operates properly in a
no-signals-based-traps environment, e.g., the module backing memory itself is regularly allocated rather than mmap'd (but is now aligned to the degree requested by the trait impl).

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:15):

cfallin requested wasmtime-default-reviewers for a review on PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:15):

cfallin edited PR #9778:

In some no_std environments, virtual memory usage is generally prohibited for performance-predictability reasons, but the MMU hardware is still in use for permissions (e.g., W^X write-xor-execute). Occasional changes to page mapping permissions are thus necessary when new modules are loaded dynamically, and are acceptable in that context. Wasmtime needs a way to support "publishing" code (making it executable) in such environments.

Rather than try to segment the signals-based-traps divide further, and piece out the code-publishing parts from the heap parts, and backdoor a path to mprotect in an otherwise no_std build, in this PR I have opted to add a trait an impl of which the embedder can provide to the Config to implement custom actions for "code publish". This otherwise operates properly in a no-signals-based-traps environment, e.g., the module backing memory itself is regularly allocated rather than mmap'd (but is now aligned to the degree requested by the trait impl).

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:28):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:28):

alexcrichton created PR review comment:

I think it might make sense to augment the previous constructor instead of adding a new one in this case as it feels reasonable to have every caller pass in the alignment

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:28):

alexcrichton created PR review comment:

WDYT about removing this feature and unconditionally including the support from this PR? I don't think LLVM will propagate that the trait object is None all that well but the cost is a few virtual dispatches in a few places which doesn't seem too bad, so this seems reasonable to me to include by default.

I ask because cargo features to me are pretty pricey in terms of maintenance due to extra CI and understanding all the #[cfg], so in the abstract we would have zero Cargo features. They're basically required for extra platform requirements though which is why we have a lot, though, but this use case isn't related (in that sense) to gating wasmtime's requirement on additional platform requirements.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:28):

alexcrichton created PR review comment:

I think it might be better to explicitly request aligned memory from the system allocator rather than performing manual alignment here. That means we can't use Vec<u8> because it won't deallocate properly, but I also think that's reasonable since we don't need any growth-related aspects of Vec here.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:32):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:32):

cfallin created PR review comment:

Unfortunately that's not an option in my embedding: memory must come from the alloc allocator.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:33):

cfallin created PR review comment:

Ah, actually, do you mean the lower-level alloc APIs? I'll look into that.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:34):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:34):

cfallin created PR review comment:

Sure, happy to fold it in; was worried the extra checks might cause some consternation but you're right, it's probably not too bad (and loading modules is relatively not a hot path).

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:36):

alexcrichton created PR review comment:

Ah yes I mean calling alloc directly and then having a custom collection which calls Drop as well (since Vec<u8> can't be used)

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:48):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 18:49):

cfallin created PR review comment:

Done.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:15):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:16):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:16):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:16):

cfallin created PR review comment:

Done.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:16):

cfallin commented on PR #9778:

Updated, thanks!

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:23):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:32):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton submitted PR review:

Would it be possible to add a test exercising this trait as well? Perhaps x86_64-linux-specific to avoid rewriting a bunch of logic to make things executable or something like that.

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton created PR review comment:

This is already stored as-is in Config so might be able to access from there (stored above) instead of also storing here too?

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton created PR review comment:

I think len can be dropped here in favor of layout.size()?

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton created PR review comment:

Would it be possible to return Option<&dyn ...> here to avoid forcing the clone on callers? Or perhaps Option<&Arc<dyn ...>> if callers should optionally have the ability to create an owned copy

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton created PR review comment:

Would SendSyncPtr suffice for this?

Wasmtime GitHub notifications bot (Dec 10 2024 at 19:52):

alexcrichton created PR review comment:

I think this may not work for the use case of combining CustomCodeMemory and compilation at runtime, so the Engine and its custom alignment might need to be plumbed down here?

Wasmtime GitHub notifications bot (Dec 10 2024 at 21:45):

github-actions[bot] commented on PR #9778:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin created PR review comment:

Ah, goodness, our crate is a veritable toy-store of useful utilities. I'm not sure how I didn't notice that -- thanks, reused!

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:27):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:28):

cfallin created PR review comment:

Ah, right, good point -- updated.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:33):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 22:39):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:21):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:37):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:38):

cfallin created PR review comment:

Done -- this was slightly gnarly to plumb through (required an extra State arg on the FinishedObject trait in environ and propagated through all the delegated builder stuff and cache handling and ...) but seems to work.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:39):

cfallin commented on PR #9778:

Updated, thanks for feedback! Added a test that uses rustix on non-Windows targets to delegate to mprotect ("custom but actually the same as always" implementation).

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:43):

cfallin has enabled auto merge for PR #9778.

Wasmtime GitHub notifications bot (Dec 10 2024 at 23:46):

cfallin updated PR #9778.

Wasmtime GitHub notifications bot (Dec 11 2024 at 00:12):

cfallin merged PR #9778.

Last updated: Dec 23 2024 at 12:05 UTC