filesystem support for solarish: stat #4031

devnexen · 2024-11-12T22:54:24Z

part of #3890

bors · 2024-11-22T07:30:38Z

☔ The latest upstream changes (presumably #4049) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung

Generally looks good, but it seems that some type is wrong somewhere, judging from CI:

  error: Undefined Behavior: scalar size mismatch: expected 2 bytes but got 4 bytes instead
    --> /Users/runner/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/pal/unix/fs.rs:1777:22
     |
  LL |         cvt(unsafe { stat64(p.as_ptr(), &mut stat) })?;
     |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ scalar size mismatch: expected 2 bytes but got 4 bytes instead

Maybe the mode has a different size on Solaris than on BSD?

src/shims/unix/fs.rs

RalfJung · 2024-11-25T21:01:53Z

unfortunately, libc crate does not contain it.

In that case please add a comment so that we can remember to add it in the future.

src/shims/unix/fs.rs

src/shims/unix/solarish/foreign_items.rs

RalfJung · 2024-11-26T06:39:52Z

tests/pass/shims/fs.rs

+    #[cfg(not(any(target_os = "solaris", target_os = "illumos")))]
    test_directory();
+    #[cfg(not(any(target_os = "solaris", target_os = "illumos")))]
    test_canonicalize();


Please use if cfg!(...) instead of #[cfg], then you can avoid making the imports so messy.

but what would be the condition tough ?

if cfg!(not(any(target_os = "solaris", target_os = "illumos"))) { ... }

but that s the thing, I tried this and the tests are expected to be called still on my illumos instance. would you like a cfg_if ? even tough a bit overkill here...

I tried this and the tests are expected to be called still on my illumos instance.

Please try again and commit the code to the PR. Sounds like there was a mistake somewhere else.

All the #[cfg] that you added elsewhere in the test should be removed.

ok will give it a go in few hours.

so, yes that was an error on my part indeed :) let s see how it goes ..

RalfJung · 2024-11-26T06:40:11Z

tests/pass/shims/fs.rs

@@ -27,7 +28,10 @@ fn main() {
    test_file_sync();
    test_errors();
    test_rename();
+    // solarish needs to intercept readdir/readdir64 for these tests.


I'd say "support" rather than "intercept"

RalfJung · 2024-11-26T06:40:28Z

src/shims/unix/solarish/foreign_items.rs

@@ -82,7 +82,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                let result = this.macos_fbsd_solaris_readdir_r(dirp, entry, result)?;
                this.write_scalar(result, dest)?;
            }
-            // TODO readdir ?
+            // TODO readdir/readdir64: for tests fs::test_directory/canonicalize


Please remove this, open an issue instead.

RalfJung · 2024-11-26T06:41:09Z

src/shims/unix/fs.rs

+        if matches!(&*this.tcx.sess.target.os, "solaris" | "illumos") {
+            let mode: u32 = metadata.mode.to_u32()?;
+
+            // FIXME: libc crate as `st_fstype` will be uninitialised
+            //this.write_int_fields_named(&[("st_mode", mode.into()), ("st_fstype", 0)], &buf)?;
+            this.write_int_fields_named(&[("st_mode", mode.into())], &buf)?;
+        } else {
+            let mode: u16 = metadata.mode.to_u16()?;
+
+            this.write_int_fields_named(&[("st_mode", mode.into())], &buf)?;
+        }


Why is this code still there?

bors · 2024-11-26T06:53:05Z

☔ The latest upstream changes (presumably #4016) made this pull request unmergeable. Please resolve the merge conflicts.

devnexen · 2024-11-26T08:22:27Z

broader question would be should we wait the libc crate update or do we keep the missing field as TODO ?

RalfJung · 2024-11-26T09:07:57Z

TODO is fine for now.

devnexen · 2024-11-26T13:01:52Z

@rustbot ready

close rust-lang#3890

RalfJung · 2024-11-26T17:23:01Z

src/shims/unix/fs.rs

+            // FIXME: libc crate, update stat struct, as `st_fstype` will be uninitialised
+            // https://github.com/rust-lang/libc/pull/4145


Suggested change

// FIXME: libc crate, update stat struct, as `st_fstype` will be uninitialised

// https://github.com/rust-lang/libc/pull/4145

// FIXME: write st_fstype field once libc is updated

// https://github.com/rust-lang/libc/pull/4145

RalfJung

@rustbot author

RalfJung · 2024-11-26T17:24:38Z

src/shims/unix/solarish/foreign_items.rs

+            "readdir_r" => {
+                let [dirp, entry, result] =
+                    this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
+                let result = this.macos_fbsd_solaris_readdir_r(dirp, entry, result)?;
+                this.write_scalar(result, dest)?;
+            }


I am confused -- why is readdir implemented now?

Please don't add untested code. If it "just works" without any other changes, enable the test, otherwise remove the implementation.

I did not add readdir**_r** it had been there since in the early phase. Anyhow, I do not need it at all for now.

RalfJung · 2024-11-26T17:49:12Z

src/shims/unix/macos/foreign_items.rs

@@ -64,7 +64,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
            "readdir_r" | "readdir_r$INODE64" => {
                let [dirp, entry, result] =
                    this.check_shim(abi, ExternAbi::C { unwind: false }, link_name, args)?;
-                let result = this.macos_fbsd_readdir_r(dirp, entry, result)?;
+                let result = this.macos_fbsd_solaris_readdir_r(dirp, entry, result)?;


Please also un-do this rename then.

RalfJung · 2024-11-26T17:49:36Z

src/shims/unix/fs.rs

+        if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd" | "solaris" | "illumos") {
+            panic!(
+                "`macos_fbsd_solaris_readdir_r` should not be called on {}",
+                this.tcx.sess.target.os
+            );


and undo this as well

RalfJung · 2024-11-26T18:06:01Z

Thanks :)

devnexen force-pushed the solarish_stat branch from 13b741d to ea9eafa Compare November 12, 2024 23:21