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

Module creation from premapped images #8245

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Milek7
Copy link
Contributor

@Milek7 Milek7 commented Mar 26, 2024

As discussed previously in #7777 for some platforms it is useful to allow loading modules using platform specific methods without mmaping memory as executable. This attempts to sidestep defining completely user-implementable CodeMemory trait, by requiring that precompiled cwasm file is mapped before using platform-specific methods, with headers and all. This way only host memory range needs to be passed to new Module::from_premapped_image method, which will then parse wasmtime-specific ELF header as usual.

For testing I hacked together this tool for packing cwasm files into Windows DLL or Linux DSO files: https://gist.github.com/Milek7/e8c1a9c284dc82c60cf48637f753b102

It can be used as follows:
wasmtime compile wasmmodule.wasm
cwasm2so pe wasmmodule wasmmodule.cwasm wasmmodule.dll

HMODULE hmodule = LoadLibraryW(L"wasmmodule.dll");
uint8_t** ptr = (uint8_t**)GetProcAddress(hmodule, "elf_ptr");
size_t* size = (size_t*)GetProcAddress(hmodule, "elf_size");

error = wasmtime_module_from_premapped_image(engine, *ptr, *size, hmodule, [](void *data) {
    FreeLibrary((HMODULE)data);
}, &module);

or
wasmtime compile wasmmodule.wasm
cwasm2so elf wasmmodule wasmmodule.cwasm wasmmodule.so

void* hmodule = dlopen("wasmmodule.so", RTLD_NOW);
uint8_t** ptr = (uint8_t**)dlsym(hmodule, "wasmmodule_elf_ptr");
size_t* size = (size_t*)dlsym(hmodule, "wasmmodule_elf_size");

error = wasmtime_module_from_premapped_image(engine, *ptr, *size, hmodule, [](void *data) {
    dlclose(data);
}, &module);

@Milek7 Milek7 requested a review from a team as a code owner March 26, 2024 21:33
@Milek7 Milek7 requested review from fitzgen and removed request for a team March 26, 2024 21:33
@github-actions github-actions bot added wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API. labels Mar 26, 2024
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api, wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@alexcrichton alexcrichton requested review from alexcrichton and removed request for fitzgen March 27, 2024 14:40
@alexcrichton
Copy link
Member

Thanks for the PR here! I like the look of this and I agree it's probably best to avoid allowing arbitrary implementations of CodeMemory. To make sure I understand what's going on here -- this is assuming that the *.so and *.dll created by your tool maps the *.cwasm into memory but the directives in the native object are such that the memory protections of all attributes are already configured appropriately? For example .text is already executable and everything else is already readonly?

Also, how willing are you to continue to work on this? I realize you're probably focused on what you're working on rather than changing this according to review, but I think some of the points below are going to be important to continue to maintain this over time for us. Additionally I think this feature could be useful to other folks as well, so I think it'd be good to polish it too if we can. That being said I'm happy to help out myself where I can, but I probably can't take on everything below, so your assistance as well would be much appreicated.

If my assumption above is correct, I like this approach! At a high-level though some things I think may want to be changed are:

  • Primarily I think we should test this in CI somehow. This sort of feature is ripe for getting broken over time so I think it would be best to have tests. This is, however, probably tricky to do because it would require your cwasm2so tool. That to me points in a slightly different direction, leading me to...
  • I think it'd be reasonable to bake in your cwasm2so into Wasmtime itself. We recently added a CodeBuilder structure which is used to configure compilation, and I think this might make a good option for that. Something like builder.create_native_so(true) or something like that so the bytes that pop out of compile_module_serialized look like the ones from cwasm2so you've written. This would, for example, set a flag in CodeBuilder where it does the normal compile process and then more-or-less runs your tool. That's what would enable running tests on CI as well (as we could perhaps literally dlopen)
  • For a more bikesheddy point, would it work for your use case to perhaps let Wasmtime dlopen the file? That way we could change the constructor to take a &Path and we'd dlopen it internally. That would help clean up the API a bit for example around the arbitrary destructor.

@Milek7
Copy link
Contributor Author

Milek7 commented Mar 27, 2024

Yes, its contents are mapped directly so that calling serialize() would yield cwasm original file, with separate segments for executable areas: (screenshots for ELF and PE, with different input files)
obraz
obraz
Here first three segments contain cwasm file, while second one corresponds to .text section inside it. Remaining segments contain metadata needed for dynamic library, headers pointing to unwind information, etc.

I have taken the view here that generating these binaries and how exactly they are loaded are up to embedder. One thing to note that while I don't need that for my use case, replacing raw memory range with library to be loaded would preclude use of linking compiled module as static library. Nevertheless if that's desired I could work on moving generation and loading into wasmtime itself some time later.

@alexcrichton
Copy link
Member

Ok makes sense, thanks for the clarification!

Personally I think it's important to have tests for this, and to do that I think it's ok to move the bits and pieces necessary to build this image into Wasmtime itself. If the bits and pieces in Wasmtime don't work for your use case though then I definitely don't want to ask you to build something you're not going to use.

One of the main worries I have is that there's a lot of implicit assumptions about the output of Wasmtime for this tool to work, so I'm a bit afraid of putting that on embedders as it seems like we may accidentally break it in the future. For example:

  • This assumes the output is an ELF, but one day we may switch to platform-specific outputs.
  • This assumes the output is a single ELF, but with components we may want to produce multiple outputs one day.
  • This assumes various specifics about the number and placement of sections.
  • This assumes that there's no relocations to be applied, which is mostly true but won't always be the case.

More-or-less I'd be more comfortable if we internalized some of these pieces in Wasmtime to be able to update it as the design in Wasmtime itself evolves over time. For example if the goal is to create a linkable object I think that'd be great to add here as well. If creating a dynamic object is all that's needed I think your gist would work well to live in Wasmtime too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants