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

Add Rust impl of wasmtime_ssp_proc_exit #125

Merged
merged 1 commit into from
Apr 27, 2019
Merged

Add Rust impl of wasmtime_ssp_proc_exit #125

merged 1 commit into from
Apr 27, 2019

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Apr 27, 2019

Replaces C implementation of wasmtime_ssp_proc_exit with pure Rust implementation. This PR is the first one of a series addressing #120.

@sunfishcode
Copy link
Member

Looks good, with one comment: Let's put the new implementations in a new file, rather than adding to host.rs. I know host.rs isn't very big right now, but its conceptual purpose is to provide types and constants and be the counterpart to wasm32.rs. Putting implementations in a separate file will help us keep the separate concerns of translating between guest/host data types and actual interface implementations separate.

As we move more things to Rust, we may eventually want to remove wasmtime_ssp.h altogether, and move what's left into host.rs -- which will be the types and the constants.

@kubkon
Copy link
Member Author

kubkon commented Apr 27, 2019

Sounds good. I'm all for good organisation of code especially if, in the future, it means simpler and cleaner maintenance.

We could either put in a separate module at the crate level, say host_impls.rs, or we could make it a submodule of the host module. In the latter case, we could then use the "import-all" trick to hide the submodule from the rest of the crate and, as a result, get away with not having to change the internals of the actual syscalls in syscalls.rs. So, to sum up, we could have it as

// in host.rs
mod impls; // or some other, more descriptive name
pub use self::impls::*;
// ...
// in host/impls.rs
use super::*;

pub fn wasmtime_ssp_proc_exit(...)

with the code in syscalls.rs unchanged. What do you reckon? Which option do you prefer?

@sunfishcode
Copy link
Member

I think I have a mild preference for having a new module at the crate level, rather than a submodule. It may even become an entirely new crate eventually.

@kubkon
Copy link
Member Author

kubkon commented Apr 27, 2019

I've moved the implementation to host_impls.rs module at the crate level. Let me know if host_impls is a good name though!

@sunfishcode
Copy link
Member

host_impls.rs works for now. Things will likely evolve over time, to be more modular, etc., but we can figure that out as we go.

@sunfishcode sunfishcode merged commit 69e44ca into bytecodealliance:master Apr 27, 2019
@kubkon
Copy link
Member Author

kubkon commented Apr 27, 2019

Hmm, analysing js-polyfill/build.sh (which I completely forgot about when submitting the PR), I think I might have been too hasty in removing the wasmtime_ssp_proc_exit from wasmtime_ssp.h. I believe that by doing so I've broken the polyfill as now __wasi_proc_exit will be missing from the exported functions table. Would you agree @sunfishcode? If so, let me know and I'll re-add the missing symbols to wasmtime_ssp.h, and posix.c. Sorry about this!

kubkon added a commit that referenced this pull request Nov 7, 2019
* Fixes `path_symlink_trailing_slashes` test case

This commit:
* adds a couple `log::debug!` macro calls in and around `path_get`
  for easier future debugging
* changes impl of `path_symlink` hostcall to actually *require*
  the final component (matching the impl of WASI in C)
* ignores the error `__WASI_ENOTDIR` in `path_get`'s `readlinkat` call
  which is not meant to be an error at this stage (i.e., this
  potentially erroneous condition *will be* handled later, in
  one of the layers above)

* Fixes `path_symlink_trailing` slashes on BSD-nixes

This commit:
* makes `path_symlink` host-specific (Linux and BSD-like nixes
  now have their own differing implementations)
* on BSD-like nixes, when `ENOTDIR` is returned from `symlinkat`
  it checks whether the target path contains a trailing slash,
  strips it, and then checks if the target path without the trailing
  slash exists; if yes, then converts the error code to `EEXIST` to
  match Linux/POSIX spec
pchickey added a commit to pchickey/wasmtime that referenced this pull request May 12, 2023
…iron_args

Make args available through an import function rather than passed to command main
pchickey added a commit to pchickey/wasmtime that referenced this pull request May 16, 2023
…iron_args

Make args available through an import function rather than passed to command main
mooori pushed a commit to mooori/wasmtime that referenced this pull request Dec 20, 2023
frank-emrich added a commit to frank-emrich/wasmtime that referenced this pull request Mar 12, 2024
…ance#125)

This PR moves the definitions of `ContinuationObject`,
`ContinuationReference`, `StackChain`, and `StackChainCell` from the
`continuations` crate to the `runtime` crate. The latter crate is the
more natural home for these types and this move is a preparation step
for merging the `wasmtime-fibre` crate into `runtime`.

The reason for not storing these types in `continuations` was that we
need access to the layout of these types from various other crates that
cannot depend on `runtime`. This layout information is provided by the
the `offsets` module in the `continuations` crate, containing the
offsets of various fields inside these types. Defining these offsets is
much easier if we have access to the involved types directly.

Now that these types are not defined in `continuations` any more, the
offsets are hard-coded instead. New tests in the `runtime` crate check
that the offset values don't go out of sync with the actual layout of
`ContinuationObject`. In the process, I've changed the types of the
offsets to `usize`. This is the more natural choice, instead of the
previous `i32`.

Moving the definitions into `runtime` also required made doc comments
mandatory on certain fields, which I've added.
avanhatt pushed a commit to wellesley-prog-sys/wasmtime that referenced this pull request Oct 9, 2024
Some of the CLIF term specs had incomplete type instantiations. This PR
populates more complete lists.

For unclear reasons this triggers some of the verification solver calls
to take a long time on 16-bit instantiations. This would need more
investigation. For now, this PR drops the solver timeout to 10 seconds
so the timeouts don't take as long.

Updates avanhatt#34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants