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

feat: add support for .so dylib files #103

Merged
merged 20 commits into from
Feb 15, 2024
Merged

Conversation

emil916
Copy link
Collaborator

@emil916 emil916 commented Sep 14, 2022

add support for .so dylib files
seperate the wasm instructions from runtime code
implement a missing wasi_intruction (prestat)

add support for .so dylib files
implement a missing wasi_intruction (prestat)
add support for .so dylib files
implement a missing wasi_intruction (prestat)
@emil916 emil916 requested a review from gparmer September 15, 2022 00:07
@emil916 emil916 added the enhancement New feature or request label Sep 15, 2022
Copy link
Collaborator

@gparmer gparmer left a comment

Choose a reason for hiding this comment

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

Looks great! only small issues around 1. wasmception, 2. clarifying the abi wording, and 3. if ifdefs are necessary or if we can just use separate compilation.

@@ -34,7 +34,8 @@ jobs:

strategy:
matrix:
libc: [wasmception, wasi-sdk]
libc: [wasi-sdk]
# libc: [wasmception, wasi-sdk]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that @Others is OK with this.

// If a function is registered using the (start) instruction, call it
WEAK void awsm_abi__start_fn() {}

#ifdef DYNAMIC_LINKING_WASM_SO
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the only way to do this with ifdefs? I thought we tried to avoid them in awsm as much as possible. Could be misremembering.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given your later code, is it accurate to say that this is the complete set of symbols that an environment depends on an awsm module for?

I don't think it captures all of the symbols that a module depends on an environment for, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got rid of the ifdef everywhere...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ended up separating the wasm instructions for all the architectures in the repo (mpx, cortex-m ...)

@emil916 emil916 requested a review from gparmer September 17, 2022 02:47
@emil916
Copy link
Collaborator Author

emil916 commented Sep 17, 2022

I did a complete new cleanup on the repo.
Got rid of all the #ifdef I added before. Instead provided a simpler Makefile flag to compile for .so lib.
Got rid of a lot of redundancy over the code base.
Made sure it is backward compatible with wasmception, too.

Sorry, it is very a lot to review I guess. But the majority is just the wat files that were missing a line and a keyword change for new version of the wabt app.

@gparmer
Copy link
Collaborator

gparmer commented Sep 17, 2022

@Others You OK with this? Just adding a runtime that can run the awsm-generated code in a .so. Useful for dynamic loading of functions, and for testing this scenario (which is what we do in sledge).

@gparmer
Copy link
Collaborator

gparmer commented Sep 21, 2022

@Others This is an increasingly useful feature to test out interesting support for extensible sandboxes. We're testing strange TLS stuff with it. What do you think about it?

@gparmer
Copy link
Collaborator

gparmer commented Sep 21, 2022

@Others: A little bit more details, and some thoughts. When we use an extern __thread... on the variable holding the memory base, function indirection array, and globals, it is generating a ton of calls to get_tls. Which means it isn't inlining those values well! This is a pretty significant performance hit. One option to handle this is to simply pass an argument to the struct containing all of these (or 3 separate variables) through all of the functions. This is equivalent, in many ways, to devoting a register to holding it. This would be a significant change to awsm, and is terrifying. But it could really help the dynamic scenario of running sandboxes that aren't LTOed with the runtime, or that use TLS.

Just a little background. Love to hear you thoughts. I know you're busy; I just wanted to write the above while it is in my head.

@emil916 emil916 merged commit f94cfea into master Feb 15, 2024
6 checks passed
@emil916 emil916 deleted the integrate-dylib-into-runtime branch February 15, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants