Implement file read/write/seek in Windows #4275
Conversation
|
@rustbot ready |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
@CraftSpider what's the status of this? A standard library test started using |
|
There's a FIXME added in rust-lang/miri-test-libstd#96 that we can hopefully remove when this lands. |
|
I'll try to get this updated soon, I've just been doing other stuff - hopefully later today or tomorrow. |
This comment has been minimized.
This comment has been minimized.
CraftSpider
force-pushed
the
windows-file-rw
branch
from
81db8b2 to
e5b2f1e
Compare
|
(modulo tests passing) |
rustbot
added
S-waiting-on-review
| // Return whether this was a success. 0 is a success. | ||
| // We wrote the return status into the IO_STATUS_BLOCK in the above callback, | ||
| // just pull it from there. |
There was a problem hiding this comment.
This doesn't work -- the entire reason that that's a callback is that desc.write may block the current thread, return immediately, and run the callback later!
This function needs to take a dest argument to capture the return place, pass that into the callback, and then write the return value that way.
There was a problem hiding this comment.
This took me a moment to get - for future reference when I'm fixing this: if the callback doesn't happen immediately, we will return the (possibly uninit) value that is in the IO_STATUS_BLOCK we were passed, then before the user code resumes, the callback will happen, write the value into IO_STATUS_BLOCK, then resume execution. We have to pass the return place into the callback to avoid that problem.
This is fairly obvious in retrospect, but it wasn't clear to me until I reread your comment a couple times.
src/shims/windows/fs.rs
Outdated
| // See NtWriteFile for commentary on this | ||
| interp_ok(this.read_scalar(&io_status)?) |
There was a problem hiding this comment.
Same here regarding the callback being potentially invoked later.
| @@ -230,19 +230,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { | |||
| let stderr = this.eval_windows("c", "STD_ERROR_HANDLE").to_i32()?; | |||
|
|
|||
| let fd_num = if which == stdin { | |||
There was a problem hiding this comment.
Please add a comment saying where those numbers come from. AFAIK those are the typical Unix numbers, right? Miri just happens to set them up always, even on Windows.
There was a problem hiding this comment.
Will do. And yeah, this is just taking advantage of the existing setup. The user won't even see these values directly, due to handle encoding.
There was a problem hiding this comment.
If we added support for SetStdHandle, then we'd need to add state to track the current values of these handles. But since we don't support that, this will just only ever return these values. Adding such support is completely possible, but not really important for std as far as I can tell.
rustbot
added
S-waiting-on-author
I can split seek off if desired, though that means I can't enable the whole file read/write shim test in
fs.rsin one go