freebsd add *stat calls interception support #3181

devnexen · 2023-11-20T20:02:02Z

No description provided.

src/shims/unix/freebsd/foreign_items.rs

RalfJung · 2023-11-21T06:34:20Z

src/shims/unix/fs.rs

-        this.assert_target_os("macos", "stat");
+
+        if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") {
+            throw_unsup_format!("macos_fbsd_stat not meant for os {}", this.tcx.sess.target.os);


This shouldn't throw a nice error, it should panic. There's a bug in Miri when this happens.

src/shims/unix/freebsd/foreign_items.rs

devnexen · 2023-11-21T20:42:13Z

@rustbot review

Cargo.lock

src/shims/unix/foreign_items.rs

RalfJung · 2023-11-23T07:54:14Z

src/shims/unix/fs.rs

-        this.assert_target_os("macos", "lstat");
+
+        if !matches!(&*this.tcx.sess.target.os, "macos" | "freebsd") {
+            panic!("macos_fbsd_lstat not meant for os {}", this.tcx.sess.target.os);


There's a verb missing in this sentence.

Suggested change

panic!("macos_fbsd_lstat not meant for os {}", this.tcx.sess.target.os);

panic!("`macos_fbsd_lstat` should not be called on {}", this.tcx.sess.target.os);

(Do this everywhere you have the new panic.)

RalfJung · 2023-11-23T07:56:23Z

src/shims/unix/fs.rs

+        }
+
+        // FIXME?: oddily it seems to pick up the old libc crate's freebsd11 definition instead of freebsd13, should be 6
+        let name_offset = if this.tcx.sess.target.os == "macos" { 5 } else { 4 };


Not sure why you mention libc here, this is a hard-coded offset, how should that pick up anything...?

Also, did that offset change between different versions of freebsd?!? How are we supposed to handle that, not knowing which version the program is built for?

Oh wait, the name is just confusing. This is not an offset! It is a field index.

This should pick up the version of libc that comes with the standard library.

after freebsd 11 dirent had been updated with the missing field (d_off) in same order than Linux (while making it cache friendly) pushing further d_name down the road.

And so the Rust libc crate then got that update as well at time point? Which libc crate version is that?

This file here should be defining which libc is picked up by Miri here. It's the latest version currently.

in fact freebsd is one of the few oses in the libc's crate which maintains several os releases versions e.g. freebsd 11 up to 14 having their own definitions when there are changes. So for our example there is dirent definition for freebsd 11 and later releases in same crate if that makes sense.

I don't quite follow, can you point me at the code for that in the libc crate? Which type does one get when asking for libc::dirent?

Sure
freebsd 11 and later

Ah, I think I found it... here. The libc build script runs some code to determine the FreeBSD version. When the FreeBSD standard library is built on a different OS (like e.g. when Miri CI tests the freebsd target on a Linux host), that will fail, and it defaults to freebsd11. But if you are using Miri on a freebsd host, it will use the host freebsd version.

Gosh, what a nightmare.^^ This is impossible to test properly.

Okay so what this means for Miri is we have to support all versions of freebsd since we can't know what the build script picks. But we can only test one of them. Amazing. Maybe long-term we want to work with the libc crate to overwrite the version detection inside Miri, though maybe that has other issues.

For this PR, it means:

definitely use project_field_named to get the name_place, that part should be easy

for writing all the other fields, do a dynamic check whether the d_off field is present. We can't test "is this freebsd or not" since the type is different for different versions of freebsd, we need to check the presence of this field. You will need to add a new helper function that checks whether a field of a given name exists.

RalfJung · 2023-11-23T07:57:58Z

src/shims/unix/fs.rs

@@ -1424,7 +1433,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                // }

                let entry_place = this.deref_pointer_as(entry_op, this.libc_ty_layout("dirent"))?;
-                let name_place = this.project_field(&entry_place, 5)?;
+                let name_place = this.project_field(&entry_place, name_offset)?;


Please use project_field_named, that should avoid this index hard-coding.

RalfJung · 2023-11-23T08:00:19Z

tests/pass/shims/fs-with-isolation.rs

@@ -1,4 +1,5 @@
 //@ignore-target-windows: File handling is not implemented yet
+//@ignore-target-freebsd


what goes wrong when this runs on freebsd? please add a FIXME

no I just forgot to enable it again..

RalfJung · 2023-11-23T08:00:45Z

tests/pass/shims/fs.rs

 use std::collections::HashMap;
+#[cfg(not(target_os = "freebsd"))]
 use std::ffi::OsString;


Please move these imports to inside test_directory rather than having such global cfg

RalfJung · 2023-11-23T08:01:20Z

tests/pass/shims/fs.rs

@@ -296,6 +300,7 @@ fn test_canonicalize() {
    remove_dir_all(&dir_path).unwrap();
 }

+#[cfg(not(target_os = "freebsd"))]


Suggested change

#[cfg(not(target_os = "freebsd"))]

#[cfg(not(target_os = "freebsd"))] // FIXME freebsd does not support readdir yet

src/shims/unix/fs.rs

RalfJung · 2023-11-23T08:06:39Z

Can you add a 32bit freebsd target to CI? With this exposure of previously 64bit-only code to 32bit targets, we definitely need that test coverage.

RalfJung · 2023-11-25T09:07:42Z

tests/pass/shims/fs.rs

@@ -296,7 +309,11 @@ fn test_canonicalize() {
    remove_dir_all(&dir_path).unwrap();
 }

+#[cfg(not(target_os = "freebsd"))] // FIXME: freebsd does not support readdir yet


Hm I just saw you implemented readdir. Why does this test fail?

RalfJung · 2023-11-25T09:08:38Z

You need to make sure that every single function that you add for freebsd is actually called by some test, on 32bit and 64bit. This is the standard we expect for all our targets. Don't just disable tests, that's now how one develops reliable software.

@rustbot review

RalfJung · 2023-11-25T09:21:15Z

Eh I meant @rustbot author.^^

Btw I am looking into why the existing libc-fs test is already failing on FreeBSD. Is it possible that ftruncate on 32bit FreeBSD takes a 64bit argument...?

RalfJung · 2023-11-25T09:22:32Z

Yes, that does seem to be the case on all BSDs. Oh wow.

RalfJung · 2023-11-25T09:33:56Z

I decided to help with the 32bit issue since it's a bit tricky to debug. #3190 makes the existing tests pass on 32bit FreeBSD, so once that lands please rebase your PR and then you only have to worry about the new functions -- and you can use the same approach for reading off_t arguments.

bors · 2023-11-25T12:06:37Z

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

devnexen · 2023-11-25T14:31:48Z

@rustbot review

RalfJung

We're getting there I think. :)

RalfJung · 2023-11-26T11:40:35Z

src/helpers.rs

@@ -225,6 +225,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
        bug!("No field named {} in type {}", name, base.layout().ty);
    }

+    /// Search if Project (which must be a struct or union type) contains the `name` field.
+    fn project_field_has_field<P: Projectable<'tcx, Provenance>>(


Suggested change

fn project_field_has_field<P: Projectable<'tcx, Provenance>>(

fn projectable_has_field<P: Projectable<'tcx, Provenance>>(

RalfJung · 2023-11-26T11:42:50Z

src/shims/unix/fs.rs

+                } else {
+                    // freebsd 12 and onwards had added the d_off field
+                    if this.project_field_has_field(&entry_place, "d_off") {


Please collapse this into } else if {

RalfJung · 2023-11-26T11:44:29Z

tests/pass/shims/fs.rs

@@ -123,7 +120,9 @@ fn test_file_create_new() {
    remove_file(&path).unwrap();
 }

+#[cfg(not(all(target_os = "freebsd", target_pointer_width = "32")))]


There's still something wrong here, why is this disabled on 32bit?
I said it multiple times: please don't just disable tests when they fail! Sometimes it is okay to cfg-out a particular test on a particular target, but that always needs a comment explaining why it is done. And here I would expect the test should pass on freebsd so when it fails that needs investigation, not cfg.

I forgot to reenable this one (especially there is no cfg upon test_seek above), no need to assume bad intentions ..

RalfJung · 2023-11-26T11:44:45Z

tests/pass/shims/fs.rs

@@ -64,6 +59,7 @@ fn test_path_conversion() {
 }

 fn test_file() {
+    use std::io::{IsTerminal, Read};


Seems unnecessary to do this for functions that are always enabled? This is only needed for cfg-gated functions.

devnexen · 2023-11-29T07:26:03Z

@rustbot review

devnexen · 2023-12-03T12:53:36Z

@saethlin I think I ve done all was asked can you have a look at it pls ?

src/helpers.rs

src/shims/unix/fs.rs

RalfJung · 2024-01-28T15:46:08Z

Sorry for the long delay!
@bors r+

bors · 2024-01-28T15:46:11Z

📌 Commit e7d4fba has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-01-28T15:47:16Z

⌛ Testing commit e7d4fba with merge d85d147...

bors · 2024-01-28T16:39:17Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing d85d147 to master...

devnexen force-pushed the stat_calls_fbsd branch from 1ca8391 to 28adab5 Compare November 20, 2023 20:35

devnexen marked this pull request as ready for review November 20, 2023 21:05

RalfJung reviewed Nov 21, 2023

View reviewed changes

src/shims/unix/freebsd/foreign_items.rs Show resolved Hide resolved

RalfJung reviewed Nov 21, 2023

View reviewed changes

src/shims/unix/freebsd/foreign_items.rs Show resolved Hide resolved

devnexen force-pushed the stat_calls_fbsd branch 7 times, most recently from 68ae0f7 to 97446f8 Compare November 21, 2023 20:07

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Nov 21, 2023

RalfJung reviewed Nov 23, 2023

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

RalfJung reviewed Nov 23, 2023

View reviewed changes

src/shims/unix/foreign_items.rs Show resolved Hide resolved

RalfJung reviewed Nov 23, 2023

View reviewed changes

src/shims/unix/fs.rs Show resolved Hide resolved

devnexen force-pushed the stat_calls_fbsd branch 6 times, most recently from a04ac72 to ffa8f29 Compare November 23, 2023 17:16

RalfJung reviewed Nov 25, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Nov 25, 2023

devnexen force-pushed the stat_calls_fbsd branch 3 times, most recently from 29c1344 to 653bcbe Compare November 25, 2023 14:31

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 25, 2023

RalfJung reviewed Nov 26, 2023

View reviewed changes

RalfJung added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Nov 26, 2023

freebsd add *stat calls interception support

6dc0f6c

devnexen force-pushed the stat_calls_fbsd branch from 653bcbe to 6dc0f6c Compare November 26, 2023 13:57

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Nov 29, 2023

RalfJung reviewed Jan 28, 2024

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

src/shims/unix/fs.rs Outdated Show resolved Hide resolved

tweak comments

e7d4fba

bors merged commit d85d147 into rust-lang:master Jan 28, 2024
8 checks passed

	panic!("macos_fbsd_lstat not meant for os {}", this.tcx.sess.target.os);
	panic!("`macos_fbsd_lstat` should not be called on {}", this.tcx.sess.target.os);

		@@ -1,4 +1,5 @@
		//@ignore-target-windows: File handling is not implemented yet
		//@ignore-target-freebsd

	#[cfg(not(target_os = "freebsd"))]
	#[cfg(not(target_os = "freebsd"))] // FIXME freebsd does not support readdir yet

	fn project_field_has_field<P: Projectable<'tcx, Provenance>>(
	fn projectable_has_field<P: Projectable<'tcx, Provenance>>(

freebsd add *stat calls interception support #3181

freebsd add *stat calls interception support #3181

Conversation

devnexen commented Nov 20, 2023

Choose a reason for hiding this comment

devnexen commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 23, 2023

Choose a reason for hiding this comment

RalfJung commented Nov 25, 2023

RalfJung commented Nov 25, 2023

RalfJung commented Nov 25, 2023

RalfJung commented Nov 25, 2023 • edited Loading

bors commented Nov 25, 2023

devnexen commented Nov 25, 2023

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devnexen commented Nov 29, 2023

devnexen commented Dec 3, 2023

RalfJung commented Jan 28, 2024

bors commented Jan 28, 2024

bors commented Jan 28, 2024

bors commented Jan 28, 2024

devnexen Nov 23, 2023 •

edited

Loading

RalfJung Nov 23, 2023 •

edited

Loading

RalfJung commented Nov 25, 2023 •

edited

Loading