Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW #1268

JOE1994 · 2020-03-27T22:39:03Z

Implemented shims for Windows

GetCurrentDirectoryW (getcwd for Windows)
SetCurrentDirectoryW (chdir for Windows)

Currently passes test :
./miri run tests/run-pass/current_dir.rs -Zmiri-disable-isolation

RalfJung · 2020-03-27T23:53:35Z

Thanks! But what about the plan to move the string helper functions to their own file?

JOE1994 · 2020-03-28T01:09:35Z

Thanks! But what about the plan to move the string helper functions to their own file?

I realized today that implementing shims for current_dir and set_current_dir on Windows was also a part of the plan laid out in #707 (comment). I was thinking of submitting a new PR to move the string helper functions to their own file, once this PR gets merged to master. Would that be okay?

RalfJung · 2020-03-28T07:54:21Z

I mean I don't have strong opinions about the order, I just feel cleanup PRs should be done before new features. And this one here adds even more string helpers, so the earlier we do this the better.

src/helpers.rs

src/shims/env.rs

RalfJung · 2020-03-28T08:06:16Z

@bors try

bors · 2020-03-28T08:06:23Z

⌛ Trying commit dfc4aaf with merge d903aa5...

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW Implemented shims for Windows * [GetCurrentDirectoryW](https://github.com/rust-lang/rust/blob/75208942f6144daac669e8e382029fc33bdce841/src/libstd/sys/windows/os.rs#L242) (`getcwd` for Windows) * [SetCurrentDirectoryW](https://github.com/rust-lang/rust/blob/75208942f6144daac669e8e382029fc33bdce841/src/libstd/sys/windows/os.rs#L250) (`chdir` for Windows) Currently passes test : `./miri run tests/run-pass/current_dir.rs -Zmiri-disable-isolation`

bors · 2020-03-28T08:26:50Z

☀️ Try build successful - checks-travis, status-appveyor
Build commit: d903aa5 (d903aa5e31afbd50f28bbc624caf209fa5fb43b3)

tests/run-pass/current_dir.rs

JOE1994 · 2020-03-28T12:34:30Z

I mean I don't have strong opinions about the order, I just feel cleanup PRs should be done before new features. And this one here adds even more string helpers, so the earlier we do this the better.

In that case, I'll submit a new PR to move the string helper functions to shims/os_str.rs, and then rebase the changes in this PR.

RalfJung · 2020-03-28T12:47:13Z

Thanks! :)

RalfJung · 2020-03-29T10:10:53Z

src/helpers.rs

@@ -428,11 +435,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                }
            })?
        } else {


Please only enter this branch if the target_os is really "windows"

RalfJung · 2020-03-29T10:11:21Z

src/helpers.rs

+                    "setting the last OS error from an io::Error is yet unsupported for {}.",
+                    target.target_os


This error makes no sense here: the target OS is fine (it's Windows), but the error is the issue. So the error should be like io error {} cannot be transformed into a raw os error above.

RalfJung · 2020-03-29T10:14:13Z

src/shims/env.rs

+                let len = this.write_path_to_wide_str(&cwd, buf, size)?.1;
+                return Ok(u32::try_from(len).unwrap())


This entirely ignores whether the buffer was big enough to hold the string (.0 component of what write_path_to_wide_str returns). Is that how Windows operates?

The docs say

If the buffer that is pointed to by lpBuffer is not large enough, the return value specifies the required size of the buffer, in characters, including the null-terminating character.

which is not what the function here does. This is the same as the "get env var" shim, so it is worth factoring out the common code into a (local, for now) helper function.

I'll fix this in the upcoming commit

RalfJung · 2020-03-29T10:14:30Z

src/shims/env.rs

+    fn SetCurrentDirectoryW (
+        &mut self,
+        path_op: OpTy<'tcx, Tag>   // LPCTSTR
+    ) -> InterpResult<'tcx, i32> { // Returns BOOL(i32 in Windows)


Suggested change

) -> InterpResult<'tcx, i32> { // Returns BOOL(i32 in Windows)

) -> InterpResult<'tcx, i32> { // Returns BOOL (i32 in Windows)

src/shims/os_str.rs

RalfJung · 2020-03-29T13:57:58Z

src/shims/os_str.rs

+/// if direction == true, Convert from 'host' to 'target'.
+/// if direction == false, Convert from 'target' to 'host'.


Please, don't use magic booleans. On the call site it is impossible to say what they mean.
Introduce an enum with appropriately named variants instead.

RalfJung · 2020-03-29T13:59:03Z

src/shims/os_str.rs

 ) -> Cow<'a, OsStr> {
    #[cfg(windows)]
    return if target_os == "windows" {
        // Windows-on-Windows, all fine.
        Cow::Borrowed(os_str)
    } else {
-        // Unix target, Windows host. Need to convert host '\\' to target '/'.
+        // Unix target, Windows host.
+        let (from, to) = if direction { ('\\', '/') } else { ('/', '\\') };


Good idea to unify the two directions into one function. :)

1. Fix 'fn convert_path_separator' in src/shims/os_str.rs 2. Fix 'fn set_last_error_from_io_error' in src/helpers.rs 3. Minor comment fix for 'fn SetCurrentDirectoryW' in src/shims/env.rs

JOE1994 · 2020-03-29T15:33:17Z

I made a force-push to rebase to master

src/shims/env.rs

src/shims/os_str.rs

RalfJung · 2020-03-29T16:43:55Z

src/shims/os_str.rs

+    }
+}
+
+enum PathConversionDirection {


Nice. :D
If you want things slightly less verbose, PathConversion also does it.
Please add a doc comment explaining the purpose of this type.

src/shims/os_str.rs

src/shims/env.rs

…s/env.rs'

RalfJung · 2020-03-29T17:19:54Z

This looks great, thanks a lot. :)
@bors r+

bors · 2020-03-29T17:19:55Z

📌 Commit 9bdb4bb has been approved by RalfJung

bors · 2020-03-29T17:20:02Z

⌛ Testing commit 9bdb4bb with merge 3504d52...

RalfJung · 2020-03-29T17:21:27Z

src/shims/os_str.rs

+        let this = self.eval_context_ref();
+        let os_str = this.read_os_str_from_wide_str(scalar)?;
+
+        Ok(PathBuf::from(&convert_path_separator(&os_str, &this.tcx.sess.target.target.target_os, Pathconversion::TargetToHost)))


This performs an unnecessary allocation when host and target are the same: you have a PathBuf, but just borrow it to convert_path_separator, which returns a Cow::Borrowed, which then PathBuf::from turns into a Cow::Owned by making a full copy.

To avoid that unnecessary copy, convert_path_separator should take a Cow<'a, OsStr> as argument. In a follow-up PR, do you want to fix that?

Please correct me if I'm wrong:
Type of os_str here is OsString (read_os_str_from_wide_str returns OsString, unlike read_os_str_from_c_str).
So I thought borrowing would not be possible, since os_str would be deallocated after control flow exits the function's local scope.

If avoiding a copy is possible here, I can work on that in a follow-up PR.

You are doing &os_str. That is borrowing.
If you use Cow, os_str will instead be moved to convert_path_separator, and this not deallocated. Then its memory can be reused when the same Cow is returned later.

Thank you for the clarification.
I'll try to fix that in a separate PR ( probably tomorrow :) )

I made a new pr to handle this issue here

bors · 2020-03-29T17:40:49Z

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

RalfJung reviewed Mar 28, 2020

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 28, 2020

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 28, 2020

View reviewed changes

src/shims/env.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 28, 2020

View reviewed changes

src/shims/env.rs Show resolved Hide resolved

RalfJung reviewed Mar 28, 2020

View reviewed changes

src/shims/env.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 28, 2020

View reviewed changes

tests/run-pass/current_dir.rs Outdated Show resolved Hide resolved

JOE1994 force-pushed the dir branch from dfc4aaf to a4b3fdd Compare March 28, 2020 18:44

RalfJung reviewed Mar 29, 2020

View reviewed changes

src/shims/os_str.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 29, 2020

View reviewed changes

JOE1994 added 5 commits March 29, 2020 11:30

partially implement 'set_last_error_from_io_error' for Windows

a481b8f

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW

1141b21

Follow-up to reviews from RalfJung

fe9ecb5

1. Fix 'fn convert_path_separator' in src/shims/os_str.rs 2. Fix 'fn set_last_error_from_io_error' in src/helpers.rs 3. Minor comment fix for 'fn SetCurrentDirectoryW' in src/shims/env.rs

fix 'magic boolean' to enum

7e0cc83

fix fn GetCurrentDirectoryW + clarify return types of Windows shims

1667ded

JOE1994 force-pushed the dir branch from 52f204a to 1667ded Compare March 29, 2020 15:32

RalfJung reviewed Mar 29, 2020

View reviewed changes

src/shims/env.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 29, 2020

View reviewed changes

src/shims/os_str.rs Outdated Show resolved Hide resolved

RalfJung reviewed Mar 29, 2020

View reviewed changes

src/shims/os_str.rs Outdated Show resolved Hide resolved

small refactorings to 'src/shims/os_str.rs' & 'src/shims/env.rs'

1b0abc5

RalfJung reviewed Mar 29, 2020

View reviewed changes

src/shims/env.rs Outdated Show resolved Hide resolved

Move definition of 'fn windows_check_buffer_size' to top of 'src/shim…

9bdb4bb

…s/env.rs'

RalfJung reviewed Mar 29, 2020

View reviewed changes

bors merged commit 3504d52 into rust-lang:master Mar 29, 2020

JOE1994 deleted the dir branch March 29, 2020 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW #1268

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW #1268

JOE1994 commented Mar 27, 2020

RalfJung commented Mar 27, 2020

JOE1994 commented Mar 28, 2020

RalfJung commented Mar 28, 2020

RalfJung commented Mar 28, 2020

bors commented Mar 28, 2020

bors commented Mar 28, 2020

JOE1994 commented Mar 28, 2020

RalfJung commented Mar 28, 2020

RalfJung Mar 29, 2020

RalfJung Mar 29, 2020

RalfJung Mar 29, 2020

JOE1994 Mar 29, 2020

RalfJung Mar 29, 2020

RalfJung Mar 29, 2020

RalfJung Mar 29, 2020

JOE1994 commented Mar 29, 2020

RalfJung Mar 29, 2020

RalfJung commented Mar 29, 2020

bors commented Mar 29, 2020

bors commented Mar 29, 2020

RalfJung Mar 29, 2020 •

edited

Loading

JOE1994 Mar 29, 2020 •

edited

Loading

RalfJung Mar 29, 2020

JOE1994 Mar 29, 2020 •

edited

Loading

JOE1994 Mar 29, 2020

bors commented Mar 29, 2020

		"setting the last OS error from an io::Error is yet unsupported for {}.",
		target.target_os

		let len = this.write_path_to_wide_str(&cwd, buf, size)?.1;
		return Ok(u32::try_from(len).unwrap())

	) -> InterpResult<'tcx, i32> { // Returns BOOL(i32 in Windows)
	) -> InterpResult<'tcx, i32> { // Returns BOOL (i32 in Windows)

		/// if direction == true, Convert from 'host' to 'target'.
		/// if direction == false, Convert from 'target' to 'host'.

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW #1268

Windows shims for GetCurrentDirectoryW/SetCurrentDirectoryW #1268

Conversation

JOE1994 commented Mar 27, 2020

RalfJung commented Mar 27, 2020

JOE1994 commented Mar 28, 2020

RalfJung commented Mar 28, 2020

RalfJung commented Mar 28, 2020

bors commented Mar 28, 2020

bors commented Mar 28, 2020

JOE1994 commented Mar 28, 2020

RalfJung commented Mar 28, 2020

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

Choose a reason for hiding this comment

JOE1994 commented Mar 29, 2020

Choose a reason for hiding this comment

RalfJung commented Mar 29, 2020

bors commented Mar 29, 2020

bors commented Mar 29, 2020

RalfJung Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

JOE1994 Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOE1994 Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 29, 2020

RalfJung Mar 29, 2020 •

edited

Loading

JOE1994 Mar 29, 2020 •

edited

Loading

JOE1994 Mar 29, 2020 •

edited

Loading