-
Notifications
You must be signed in to change notification settings - Fork 432
Add a shim for uname systemcall #4784
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
|
Thanks for the PR! Please extend the PR description, explaining the motivation for why adding this syscall is helpful and why it makes sense to return some hard-coded "dummy" data. |
|
This is my first contribution to Miri, so I probably did something not quite right. If so, please let me know. I tried to follow the conventions I found in the code as the contribution guide didn't provide a lot of guidance around adding support for new system calls. |
Seems like most the BSD family (FreeBSD, OpenBSD, NetBSD, macOS) doesn't have this fields, only Linux.
x86_64 uses i8, aarch64 seems to use u8.
|
Updated the pr description. I think I also solved most issues found by the CI, but the Windows one I'm not sure of. In |
|
In general, it is wrong to implement shims in Miri by just calling the shimmed function in the shim, which is what you are trying to do. Miri is a cross-interpreter, so it needs to be able to run Linux code on Windows. Ideally, you implement a shim by calling a portable function in the standard library. If that is not available, you might be able to use |
src/shims/unix/env.rs
Outdated
| return this.set_last_error_and_return_i32(LibcError("EFAULT")); | ||
| } | ||
|
|
||
| let mut uname_buf = unsafe { std::mem::zeroed() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, we try to avoid unsafe in the interpreter except where it is absolutely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit to initialise all fields to zero. Do note that will make porting to new OS, which might have a different set of fields in utsname, a bit more annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't use any libc types or functions here. Those correspond to the current host, which is entirely unrelated to the current target.
I think we should avoid this -- I don't want Miri to become a platform abstraction layer full of For uname, if we do have a shim at all, it should probably just return something plausible that is hard-coded. But I can easily see that causing more trouble than it solves. What kernel features does a10 enable based on the kernel version? Chances are, Miri doesn't support them anyway. Everything we support is quite ancient, AFAIK. So it's probably best to just hard-code some old kernel version under |
|
Looking at the code, this seems to be mostly about io_uring... and sure enough, a10 is an io_uring library. Miri is quite far from having any sort of io_uring support (see #4636). So even if uname works, I'm afraid Miri is not going to get very far in your test suite. I appreciate the attempt to help, but I'm afraid Miri is not very suited for such super low-level platform-specific code. |
It doesn't change the code based on the kernel version, it only changes the tests to skip them if a certain feature is not supported and the OS would return some kind of not supported error.
To clear up any potential confusion, This pr is an experiment for myself to get more familiar with Miri's codebase, I thought I do so while getting slightly closer to getting A10's test suite to run in Miri. The end goal for me would be to contribute support for io_uring (but I'm well aware that's a lot more work than this pr :) ). Re: making the system call vs. hardcoded values. Let me know what is the best route here, I don't know enough about Miri's internals & goals to argue one way or the other. |
|
Related prior work: #2494. We ended up not implementing that since it wouldn't really have helped for any usecase. |
|
Thinking a bit more specifically about this case -- IMO it makes little sense to query the kernel version of the host and use it in the interpreted program. I can't imagine any way of using
I understand it has nothing to do with io_uring. :) But the usecase you presented is "I have code that checks the kernel version to see which io_uring flags are supported" (if I understood it correctly). I am not entirely convinced that's a sufficient motivation, for the reasons stated above. However, "it is part of POSIX" is a motivation I could be convinced of. However, if we give any kind of version number, we have to be a bit careful -- your current implementation uses something involving "Linux" and "6.18" on all Unix targets, which makes little sense for e.g. FreeBSD or Illumos. You're also setting Wouldn't it be better to not even try to make this look like a real Linux, and instead just say "this is Miri 0.1" (based on @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
So we don't pretend to be Linux when running in isolation.
No, as I said above I think that does not make sense. That should then also avoid the CI issues. @rustbot author |
The domainname field is only available on Linux and Android. For OS that don't have this field simply ignore the value.
This is used by FreeBSD to implement uname.
|
This required a bit more work:
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We are entering the fine-tuning phase of this PR. :)
| let result = this.getpid()?; | ||
| this.write_scalar(result, dest)?; | ||
| } | ||
| "uname" => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently that's not universally the symbol name then, given what you had to do for freebsd? Please add a check similar to what we do for flock and other shims to ensure this arm is only taken on OSes where it makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is the correct name for the function, but the implementation for FreeBSD within libc then calls __xuname. I'm not exactly sure what would happen if we remove it from the list here, but I think it would cause errors to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried running it with uname here and with --target x86_64-unknown-freebsd and it does work, then I'll have to figure what the correct names are
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This match arm here is just dead code for freebsd target. So just add a check to only allow this arm on the other Unixes.
| ("sysname", "Miri"), | ||
| ("nodename", "Miri"), | ||
| ("release", env!("CARGO_PKG_VERSION")), | ||
| ("version", ""), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does "version" usually contain and why is it empty here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, but I think it's very much OS dependant. Posix defines it as (https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_utsname.h.html):
Current version level of this release.
Some examples:
- Arch Linux:
#1 SMP PREEMPT_DYNAMIC Thu, 18 Dec 2025 18:00:18 +0000 - macOS:
Darwin Kernel Version 24.6.0: Wed Oct 15 21:12:15 PDT 2025; root:xnu-11417.140.69.703.14~1/RELEASE_ARM64_T6041
I'm open to suggestion on what to put here for Miri, but realistically I don't think this info is useful in general (outside from maybe logging it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand the distinction between "release" and "version"... but it seems like typically, both contain a version number. So maybe "version" should be "Miri 0.1" or so?
src/shims/unix/env.rs
Outdated
| // Since not all OS have all fields (e.g. domainname), we ignore | ||
| // fields that are not present. | ||
| if let Some(field) = this.try_project_field_named(&uname, name)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is domainname the only exception here?
The usual approach we take is to assert that the fields we expect to exist do exist. See fn write_stat_buf. Seems best to remain consistent and follow the same approach here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is domainname the only exception here?
Yes. It's not the Posix standard, it seems like a glibc/GNU addition.
The usual approach we take is to assert that the fields we expect to exist do exist. See
fn write_stat_buf. Seems best to remain consistent and follow the same approach here.
I'm not sure if the approach in write_stat_buf is a good idea to be honest. It would mean a higher cost when porting to now OS as you need to check if the OS has the field or not. Furthermore if we don't fill the field for an OS that does define it will cause UB if the user didn't initialise the buffer before the call.
I've split the values in required fields (using project_field_named) and optional fields (using the try_ variant, still ignoring if it's missing). Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine... I'm not a huge fan of try_project_field_named even existing but OTOH we already gave it and use it elsewhere (though not very often). The better API here would somehow check that we did not miss a field but until we have that, we can go with the "try" variant.
Keeping the domainname as optional as it seems like only GNU/Linux uses this field.
This system call is used to get information about the OS that is run. A10 for example uses this to determine what Linux version is being run and uses it to skip tests that are not supported by older Linux versions, see https://github.com/Thomasdezeeuw/a10/blob/87d04713a3ff5f9d60e1473a72939a1e1bcdaf13/tests/util/mod.rs#L44-L62.
When Miri runs in isolation mode this returns some dummy data based on a recent Linux version. Since the data should only be informative I think it makes sense to return something and allow the program to continue rather than stopping it.