Stream: git-wasmtime

Topic: wasmtime / PR #5173 cranelift: improve syscall error/oom ...

Wasmtime GitHub notifications bot (Nov 02 2022 at 00:14):

11evan opened PR #5173 from jit-system-errors to main:

The JIT module has several places where it expects or panics on syscall or allocator errors. For example, mmap and mprotect can fail if Linux vm.max_map_count is not high enough, and some users may wish to handle this error rather than immediately crashing.

This commit plumbs these errors upward as new ModuleError types, so that callers of jit module functions like finalize_definitions and define_function can handle them (or just unwrap(), as desired).

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 02 2022 at 10:48):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 10:48):

bjorn3 created PR review comment:

I think this one should be placed behind the Backend variant.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:14):

11evan updated PR #5173 from jit-system-errors to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:18):

11evan created PR review comment:

Do you mean it should be reordered to come after Backend? Or does "behind" mean a multi level error enum like

enum BackendError {
    Allocation { ... }
    Syscall { ... }
    Other { ... }
}

?

Wasmtime GitHub notifications bot (Nov 02 2022 at 16:18):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:10):

11evan updated PR #5173 from jit-system-errors to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:51):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 17:51):

bjorn3 created PR review comment:

No, I mean the anyhow::Error field of ModuleError::Backend should be used to store this error I think. anyhow::Error allows storing any type implementing std::error::Error, including the std::io::Error you have here.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:15):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:15):

11evan created PR review comment:

Ah, I see. Yeah, anyhow::Error can store an io::Error, and we could use it to get rid of the Allocation and Syscall error variants, and use ModuleError::Backend for all cases.

The difference is that the separate variants make it possible for the code catching the error to distinguish between allocation/syscall and other failures, since it can match and tell them apart. If all cases are anyhow::Error, the caller can't really disambiguate an allocation failure, from a syscall failure, from other Backend errors that already exist.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:18):

bjorn3 created PR review comment:

For allocation failure having a separate ModuleError variant makes sense to me. For syscall failure, I don't think you would ever want to distinguish it from any other io::Error, but if that is necessary it would be possible to add a new error type to cranelift-jit wrapping io::Error in that case and then store this new type in anyhow::Error.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:18):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:42):

11evan updated PR #5173 from jit-system-errors to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:46):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:46):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 18:46):

bjorn3 created PR review comment:

This may not necessarily be correct. Not every global allocator impl sets errno. In fact I believe most don't.

Wasmtime GitHub notifications bot (Nov 02 2022 at 19:10):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 19:10):

11evan created PR review comment:

Ah, I didn't realize. Maybe it would be better to just return io::Error::from(io::ErrorKind::OutOfMemory) ourselves here, then?

Wasmtime GitHub notifications bot (Nov 02 2022 at 19:24):

11evan updated PR #5173 from jit-system-errors to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 20:15):

bjorn3 created PR review comment:

Yeah, that makes sense.

Wasmtime GitHub notifications bot (Nov 02 2022 at 20:15):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 16:22):

11evan submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 16:23):

11evan created PR review comment:

Updated to directly return OutOfMemory instead of trying to use last_os_error()

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:59):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 23:59):

abrown merged PR #5173.

Last updated: Nov 22 2024 at 16:03 UTC