Skip to content
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

add stat shim for macos #1129

Merged
merged 13 commits into from
Dec 27, 2019
Merged

add stat shim for macos #1129

merged 13 commits into from
Dec 27, 2019

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Dec 24, 2019

Here we go. I'm apologizing in advice for the constant torture that Travis will suffer.

@pvdrz pvdrz force-pushed the stat64-shim branch 2 times, most recently from 892a022 to 462220d Compare December 24, 2019 19:53
@pvdrz pvdrz changed the title add stat64 shim add stat shim for macos Dec 24, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Dec 24, 2019

Maybe we should set up a docker or sth that allows ppl to build for arbitrary architectures

@pvdrz
Copy link
Contributor Author

pvdrz commented Dec 25, 2019

Maybe we should set up a docker or sth that allows ppl to build for arbitrary architectures

I'm trying something crazy to avoid abusing Travis. Let's hope it works.

@RalfJung
Copy link
Member

What would also help for cross-paltform testing, I think, is #1057. Then you could just cross-compile a macOS libstd on a Linux host, and run Miri in macOS target mode. At least that's my hope. ;)

@pvdrz
Copy link
Contributor Author

pvdrz commented Dec 25, 2019

I have a misalignment somewhere. These are the values in hexadecimal of the fields that I'm able to see right now:

                       macos            expected
dev:                00000001            00000001 i32
mode:                   8000                8000 u16
nlink:                  0002                0002 u16
ino:        0000000000000003    0000000000000003 u64
uid:                00000004            00000004 u32
gid:                00000005            00000005 u32
rdev:               00000006            00000006 i32
atime:      0000000000000000    000000005e037125 i64
atime_nsec: 5e03712500000000    0000000000000000 i64
mtime:      0000000000000000    000000005e037125 i64
mtime_nsec: 0000000700000000    0000000000000000 i64
ctime:      0000000800000000    0000000000000007 i64
ctime_nsec: 5e03712500000000    0000000000000008 i64
btime:      ????????????????    000000005e037125 i64
btime_nsec: ????????????????    0000000000000000 i64
size:       0000000900000000    000000000000000e i64
blocks:     0000000a00000000    0000000000000009 i64
blksize:            0000000a            0000000a i32

Edit: It seems the culprit was the alignment when starting to write the times. All the fields before the times have a size that is an odd multiple of 32. I added a padding of 32 extra bits and it seems that everything got aligned correctly.

Edit2: of course this doesn't happen on 32-bit platforms... so i686 failed. I'm trying to fix it now.

@pvdrz pvdrz force-pushed the stat64-shim branch 2 times, most recently from c204828 to a5411ba Compare December 25, 2019 16:50
@pvdrz
Copy link
Contributor Author

pvdrz commented Dec 25, 2019

@RalfJung I'm very close to finishing the implementation of stat. Sadly the terminfo problem continues :(. Now the test that failed when TERM was set, fails because it cannot close the file.

It seems I made a mistake long ago when I implemented close. There is no way call close from libstd and actually recover the os error in a single call. I thought that I had solved that by calling File::sync_all. But after checking the implementation of File::sync_all it actually calls fcntl(_, F_FULLFSYNC) which is not what we want (and apparently is not implemented in this platform).

My best guess would be to simply drop the file and then call std::io::Error::last_os_error. But even then that would only work if closing actually fails. Another option would be calling close directly but then we'd need to implement something different for windows. I'll keep excluding TERM for now and think what to do with close.

Here are the logs of the travis build: https://travis-ci.com/rust-lang/miri/jobs/270303147#L1029

@pvdrz pvdrz marked this pull request as ready for review December 25, 2019 18:09
@RalfJung
Copy link
Member

Let's ignore TERM for now and focus on stat, then.

@pvdrz
Copy link
Contributor Author

pvdrz commented Dec 25, 2019

Should I try to deduplicate the shared code between statx and stat? We could do our own "stat" structure and grow it slowly.

@RalfJung
Copy link
Member

Should I try to deduplicate the shared code between statx and stat? We could do our own "stat" structure and grow it slowly.

That seems reasonable. Do you want to do that in this PR or a follow-up?

@pvdrz
Copy link
Contributor Author

pvdrz commented Dec 25, 2019

I have something ready in local. Let me clean it

@RalfJung
Copy link
Member

LGTM, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 27, 2019

📌 Commit c8190e8 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Dec 27, 2019

⌛ Testing commit c8190e8 with merge 0f1dec0...

bors added a commit that referenced this pull request Dec 27, 2019
add stat shim for macos

Here we go. I'm apologizing in advice for the constant torture that Travis will suffer.
@bors
Copy link
Contributor

bors commented Dec 27, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 0f1dec0 to master...

@bors bors merged commit c8190e8 into rust-lang:master Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants