Support DirEntry metadata calls in miri #103075

SUPERCILEX · 2022-10-15T03:53:04Z

This should work as it uses lstat64 which is supported here: ~~https://github.com/rust-lang/miri/blob/d9ad25ee4bbd9364c498959cdc82b5fa6c41e63c/src/shims/unix/macos/foreign_items.rs#L42~~ just noticed that's macos, linux would be using statx: https://github.com/rust-lang/miri/blob/86f0e63b21721fe2c14608644f467b9cb21945eb/src/shims/unix/linux/foreign_items.rs#L112

The failing syscall is dirfd, so maybe that should actually be added to the shims?

rust-highfive · 2022-10-15T03:53:07Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-10-15T03:53:07Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

SUPERCILEX · 2022-10-15T03:53:16Z

r? @RalfJung

RalfJung · 2022-10-15T06:31:06Z

I can't approve miri-specific library hacks, that needs someone from the libs team.
Cc @thomcc

However, from the Miri side, this is lacking tests that ensure that this actually works.

The failing syscall is dirfd, so maybe that should actually be added to the shims?

The problem is that dirfd is not exposed by the standard library, so Miri has no good way of implementing that shim in a cross-platform way.

thomcc · 2022-10-15T06:44:02Z

r? @thomcc

thomcc · 2022-10-15T06:44:51Z

The problem is that dirfd is not exposed by the standard library, so Miri has no good way of implementing that shim in a cross-platform way.

This would bother me more if we didn't already have both code-paths -- maintaining the miri in the cfg does not worry me.

It is unfortunate from the perspective of having slightly inaccurate emulation on these targets, though.

However, from the Miri side, this is lacking tests that ensure that this actually works.

Hmm, this is a good point I guess. Would adding a test for this function in libstd be sufficient here?

RalfJung · 2022-10-16T07:43:23Z

Which libstd API function is enabled by this? The test should be added in src/tools/miri/tests/pass-dep/shims/fs.rs.

thomcc · 2022-10-16T17:42:23Z

@rustbot author

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

rustbot · 2022-10-16T19:14:37Z

The Miri subtree was changed

cc @rust-lang/miri

SUPERCILEX · 2022-10-16T19:45:56Z

@rustbot ready

thomcc · 2022-10-16T20:37:41Z

Thanks. Looks good to me if @RalfJung doesn't want any changes to the tests.

oli-obk · 2022-10-18T10:05:01Z

@bors r=thomcc

Looks good to me

bors · 2022-10-18T10:05:03Z

📌 Commit 7273358 has been approved by thomcc

It is now in the queue for this repository.

bors · 2022-10-18T10:54:56Z

⌛ Testing commit 7273358 with merge 21b2465...

bors · 2022-10-18T13:35:36Z

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 21b2465 to master...

rust-timer · 2022-10-18T18:11:47Z

Finished benchmarking commit (21b2465): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.7%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

the arithmetic mean of the percent change ↩
number of relevant changes ↩

RalfJung · 2022-10-19T11:55:22Z

src/tools/miri/tests/pass-dep/shims/fs.rs

@@ -404,6 +404,14 @@ fn test_directory() {
    let mut file_names = dir_iter.map(|e| e.unwrap().file_name()).collect::<Vec<_>>();
    file_names.sort_unstable();
    assert_eq!(file_names, vec!["test_file_1", "test_file_2"]);
+    // Test that read_dir metadata calls succeed
+    assert_eq!(
+        &[true, true],


Ideally we'd also make sure it returns false on directories.

Sounds good, created #103258

Support DirEntry metadata calls in miri This should work as it uses lstat64 which is supported here: ~https://github.com/rust-lang/miri/blob/d9ad25ee4bbd9364c498959cdc82b5fa6c41e63c/src/shims/unix/macos/foreign_items.rs#L42~ just noticed that's macos, linux would be using statx: https://github.com/rust-lang/miri/blob/86f0e63b21721fe2c14608644f467b9cb21945eb/src/shims/unix/linux/foreign_items.rs#L112 The failing syscall is `dirfd`, so maybe that should actually be added to the shims?

rust-highfive assigned Mark-Simulacrum Oct 15, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2022

rust-highfive assigned RalfJung and unassigned Mark-Simulacrum Oct 15, 2022

SUPERCILEX force-pushed the miri-metadata branch from c80ec9b to 7ec8984 Compare October 15, 2022 05:18

rust-highfive assigned thomcc and unassigned RalfJung Oct 15, 2022

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2022

Support DirEntry metadata calls in miri

7273358

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX force-pushed the miri-metadata branch from 7ec8984 to 7273358 Compare October 16, 2022 19:14

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 18, 2022

bors merged commit 21b2465 into rust-lang:master Oct 18, 2022

rustbot added this to the 1.66.0 milestone Oct 18, 2022

SUPERCILEX deleted the miri-metadata branch October 18, 2022 19:26

RalfJung reviewed Oct 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DirEntry metadata calls in miri #103075

Support DirEntry metadata calls in miri #103075

SUPERCILEX commented Oct 15, 2022 •

edited

Loading

rust-highfive commented Oct 15, 2022

rustbot commented Oct 15, 2022

SUPERCILEX commented Oct 15, 2022

RalfJung commented Oct 15, 2022

thomcc commented Oct 15, 2022

thomcc commented Oct 15, 2022

RalfJung commented Oct 16, 2022

thomcc commented Oct 16, 2022

rustbot commented Oct 16, 2022

SUPERCILEX commented Oct 16, 2022

thomcc commented Oct 16, 2022

oli-obk commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 18, 2022

rust-timer commented Oct 18, 2022

RalfJung Oct 19, 2022 •

edited

Loading

SUPERCILEX Oct 19, 2022

Support DirEntry metadata calls in miri #103075

Support DirEntry metadata calls in miri #103075

Conversation

SUPERCILEX commented Oct 15, 2022 • edited Loading

rust-highfive commented Oct 15, 2022

rustbot commented Oct 15, 2022

SUPERCILEX commented Oct 15, 2022

RalfJung commented Oct 15, 2022

thomcc commented Oct 15, 2022

thomcc commented Oct 15, 2022

RalfJung commented Oct 16, 2022

thomcc commented Oct 16, 2022

rustbot commented Oct 16, 2022

SUPERCILEX commented Oct 16, 2022

thomcc commented Oct 16, 2022

oli-obk commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 18, 2022

bors commented Oct 18, 2022

rust-timer commented Oct 18, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

RalfJung Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

SUPERCILEX Oct 19, 2022

Choose a reason for hiding this comment

SUPERCILEX commented Oct 15, 2022 •

edited

Loading

RalfJung Oct 19, 2022 •

edited

Loading