Update syscall handlers to take explicit argument types #2664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Component: Main
Codecov ReportBase: 68.18% // Head: 68.06% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2664 +/- ##
==========================================
- Coverage 68.18% 68.06% -0.12%
==========================================
Files 202 202
Lines 30401 30407 +6
Branches 5935 5938 +3
==========================================
- Hits 20730 20698 -32
- Misses 4977 5012 +35
- Partials 4694 4697 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
stevenengler
force-pushed
the
syscall-handler-extractors
branch
from
ac355fe
to
10c97fc
Compare
github-actions
bot
added
the
Component: Libraries
This allows us to write the syscall handlers to take explicit argument types.
For example the previous
```rust
pub fn dup3(ctx: &mut SyscallContext, args: &SysCallArgs) -> SyscallResult {
let old_fd = libc::c_int::from(args.get(0));
let new_fd = libc::c_int::from(args.get(1));
let flags = libc::c_int::from(args.get(2));
[...]
}
```
becomes
```rust
pub fn dup3(
ctx: &mut SyscallContext,
old_fd: libc::c_int,
new_fd: libc::c_int,
flags: libc::c_int,
) -> SyscallResult {
[...]
}
```
stevenengler
force-pushed
the
syscall-handler-extractors
branch
from
10c97fc
to
52f016c
Compare
This was referenced Feb 17, 2023
stevenengler
added a commit
that referenced
this pull request
Feb 23, 2023
Similar to #2664 but for return types. Syscalls can return various types (`int`, `size_t`, pointers, etc). We probably don't want to return the wrong type, which the plugin may interpret incorrectly. For example if a syscall is supposed to return an `int`, we probably don't want to return a `u64::MAX` from our syscall handler. Currently our syscall handlers return a `SysCallReg`, which is a 64-bit union of various types, and there is no type checking to make sure our syscall handler is returning a valid type. This PR allows our syscall handlers to specify an explicit return type (ex: `libc::c_int` instead of `SysCallReg`). Only the "sched.h" syscall handlers have been updated to use this. I didn't bother with the others. We can update them later if we want to, but for now this change allows us to use this for new syscall handlers that we write in the future. (Related #2658.) Before: ```rust pub fn sched_yield(_ctx: &mut SyscallContext) -> SyscallResult { Ok(0.into()) } ``` After: ```rust pub fn sched_yield(_ctx: &mut SyscallContext) -> Result<libc::c_int, SyscallError> { Ok(0) } ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently our syscall handlers are written as:
This has the downside that we need to convert the
SysCallRegvalues to the correct type. And if we don't do so explicitly like in the example above, Rust may use type inference and convert it to the wrong type (see discussion in #2658). It would be nice if we could force syscalls to provide the types explicitly in the function arguments:With these explicit types in the function arguments it's still possible to provide an incorrect type, but would prevent type-inference mistakes and hopefully would make the syscall handlers easier to check.