11evan opened PR #5173 from jit-system-errors
to main
:
The JIT module has several places where it
expect
s orpanic
s on syscall or allocator errors. For example,mmap
andmprotect
can fail if Linuxvm.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 likefinalize_definitions
anddefine_function
can handle them (or justunwrap()
, 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.
-->
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I think this one should be placed behind the
Backend
variant.
11evan updated PR #5173 from jit-system-errors
to main
.
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 likeenum BackendError { Allocation { ... } Syscall { ... } Other { ... } }
?
11evan submitted PR review.
11evan updated PR #5173 from jit-system-errors
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
No, I mean the
anyhow::Error
field ofModuleError::Backend
should be used to store this error I think.anyhow::Error
allows storing any type implementingstd::error::Error
, including thestd::io::Error
you have here.
11evan submitted PR review.
11evan created PR review comment:
Ah, I see. Yeah,
anyhow::Error
can store anio::Error
, and we could use it to get rid of theAllocation
andSyscall
error variants, and useModuleError::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 areanyhow::Error
, the caller can't really disambiguate an allocation failure, from a syscall failure, from otherBackend
errors that already exist.
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 otherio::Error
, but if that is necessary it would be possible to add a new error type to cranelift-jit wrappingio::Error
in that case and then store this new type inanyhow::Error
.
bjorn3 submitted PR review.
11evan updated PR #5173 from jit-system-errors
to main
.
bjorn3 submitted PR review.
bjorn3 submitted PR review.
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.
11evan submitted PR review.
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?
11evan updated PR #5173 from jit-system-errors
to main
.
bjorn3 created PR review comment:
Yeah, that makes sense.
bjorn3 submitted PR review.
11evan submitted PR review.
11evan created PR review comment:
Updated to directly return
OutOfMemory
instead of trying to uselast_os_error()
abrown submitted PR review.
abrown merged PR #5173.
Last updated: Dec 23 2024 at 12:05 UTC