-
Notifications
You must be signed in to change notification settings - Fork 414
feat: Support fstat in linux
#4714
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. |
e009ee2 to
8731b63
Compare
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 for the PR! However, there are some things missing.
| "fstat" => { | ||
| let [fd, buf] = this.check_shim_sig_lenient(abi, CanonAbi::C, link_name, args)?; | ||
| let result = this.fstat(fd, buf)?; | ||
| this.write_scalar(result, dest)?; | ||
| } |
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.
As explained in the issue, this should be moved into src/shims/unix/foreign_items.rs, and then the corresponding blocks in the other unixes can be removed. (Some of them also make the function available under other names; those cannot be removed.)
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 to remove them from other unixes but it seems all of them are under different names
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 can add fstat64 here as well.
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 didn't yet add fstat64 here. What was your plan?
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.
Hm, if we are fstat64 we should probably use the stat64 type, not the stat type... but somehow this worked before? Not entirely sure what is happening. This might be related to the freebsd test failure.
|
Reminder, once the PR becomes ready for a review, use |
|
Please use However, it seems like there are some CI failures that need to be dealt with before this is ready? |
refactor: rename `macos_fbsd_solarish_write_stat_buf` to `write_stat_buf` refactor: rename `macos_fbsd_solarish_fstat` to `fstat` feat: support `fstat` in linux test: testing support of `fstat` in linux fix: missed add `Os::Linux` for supported OSs in `fstat` feat: add nanosecond fields to file metadata in `EvalContextExtPrivate` add `fstat` to foreign items in unix enhance test of `fstat` fix the test
620531b to
d2ded9c
Compare
|
@rustbot ready |
|
for some reason, it fails on maybe the problem comes from #[repr(C)]
struct SafeStat {
stat: libc::stat,
_pad: [u64; 16],
}
let mut safe_stat: SafeStat = unsafe { MaybeUninit::zeroed().assume_init() };
let stat_ptr = &mut safe_stat as *mut SafeStat as *mut libc::stat;
let res = unsafe { libc::fstat(fd, stat_ptr) };what do you think? |
|
Note that the target it failed on is x86_64-unknown-freebsd. So you are looking at the wrong target. We definitely should not do anything like that in the test. If a test fails, usually that means you have to fix the code, not the test. |
| let result = this.macos_fbsd_solarish_lstat(path, buf)?; | ||
| this.write_scalar(result, dest)?; | ||
| } | ||
| "fstat" | "fstat64" => { |
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.
As I have already said twice, this match arm should be removed now. Please make sure you understand my comments before marking the PR as ready, and ask if anything is unclear. Do not just guess and update the PR. Only mark it as ready once you are confident everything is ready. It's a lot of extra work for me if I have to keep re-checking and repeating everything I already said.
| ("st_mtime", modified_sec.into()), | ||
| ("st_mtime_nsec", modified_nsec.into()), | ||
| ("st_ctime", 0), | ||
| ("st_ctime_nsec", 0), |
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 are now writing these fields twice on macos and freebsd.
| let result = this.macos_fbsd_solarish_lstat(path, buf)?; | ||
| this.write_scalar(result, dest)?; | ||
| } | ||
| "fstat" | "fstat@FBSD_1.0" => { |
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.
| "fstat" | "fstat@FBSD_1.0" => { | |
| "fstat@FBSD_1.0" => { |
As elsewhere, the "fstat" case should be removed here, since it is already handled in the "unix" handler now.
|
I looked into the BSD issue. Apparently on BSD there is The fix for us is fairly simple though -- we just trust the type signature used by the program, since double-checking it is apparently impractically difficult: diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs
index 16214d7ef..c33100b24 100644
--- a/src/shims/unix/fs.rs
+++ b/src/shims/unix/fs.rs
@@ -130,7 +130,11 @@ trait EvalContextExtPrivate<'tcx>: crate::MiriInterpCxExt<'tcx> {
let (modified_sec, modified_nsec) = metadata.modified.unwrap_or((0, 0));
let mode = metadata.mode.to_uint(this.libc_ty_layout("mode_t").size)?;
- let buf = this.deref_pointer_as(buf_op, this.libc_ty_layout("stat"))?;
+ // We do *not* use `deref_pointer_as` here since determining the right pointee type
+ // is highly non-trivial: it depends on which exact alias of the function was invoked
+ // (e.g. `fstat` vs `fstat64`), and then on FreeBSD it also depends on the ABI level
+ // which can be different between the libc used by std and the libc used by everyone else.
+ let buf = this.deref_pointer(buf_op)?;
this.write_int_fields_named(
&[
("st_dev", metadata.dev.into()), |
Close #4684