mir-interpret: add method to read wide strings from Memory

[JOE1994]

Implemented step2 from instructions laid out in rust-lang/miri#707.

Added 2 new methods to struct rustc_mir::interpret::InterpCx.

• read_os_str_from_wide_str (src/librustc_mir/interpret/operand.rs)
• write_os_str_to_wide_str (src/librustc_mir/interpret/place.rs)
  • used existing logic implemented in MIRI/src/eval.rs

These methods are intended to be used for environment variable emulation in Windows.

[rust-highfive]

r? @ecstatic-morse

(rust_highfive has picked a reviewer for you, use r? to override)

[JOE1994]

r? @RalfJung

[JOE1994]

I have some implementation questions..

1. Current return type of read_os_str_from_wide_str is InterpResult<'tcx, std::ffi::OsString>. Is that okay as it is? Or should it instead be InterpResult<'tcx, &std::ffi::OsStr> (similar to the method read_str which returns InterpResult<'tcx, &str>)

2. write_os_str_to_wide_str inserts a NULL terminator to the string to be stored. Currently, read_os_str_from_wide_str doesn't use a NULL terminator to check the length(or termination) of a string. In this case, inserting a NULL terminator, is only being done to mimic behavior of what would be done in Windows. I haven't yet written the NULL terminator discarding logic in read_os_str_from_wide_str.
   I'm not sure whether I should

   • Add NULL terminator discarding logic in read_os_str_from_wide_str.
     OR
   • Remove NULL terminator inserting logic in write_os_str_to_wide_str.

Thank you for reading 👍

[tesuji]

If the conversion is for Windows only, consider https://doc.rust-lang.org/nightly/std/ffi/index.html#on-windows

[RalfJung]

read_os_str_from_wide_str doesn't use a NULL terminator to check the length(or termination) of a string

That is pretty odd, and why should that work? The users of these APIs have no idea how long the string is going to be.

You used mplace.len(), that makes little sense as it assumes that this is a slice or array. But the case where you need this method is when you got a raw ptr to some memory, and you have no idea how long the string is. That is all you got in the Windows APIs you want to implement: you have a place containing the pointer, which you read to get the pointer, and then you have to work with that. You don't get a place containing the string.

Current return type of read_os_str_from_wide_str is InterpResult<'tcx, std::ffi::OsString>. Is that okay as it is? Or should it instead be InterpResult<'tcx, &std::ffi::OsStr>

There should be nothing about OsStr or OsString on the rustc side, only Miri needs this. On the Miri side, InterpResult<'tcx, std::ffi::OsString> seems like a reasonable return type for read_os_str_from_wide_str. We cannot do &OsStr because we cannot directly use the underlying storage; that is okay.

For in this PR, you only need to add on the rustc side whatever primitives are needed to implement read_os_str_from_wide_str and write_os_str_to_wide_str in Miri. And I think the only primitive you need is Memory::read_wide_c_str, which should be like Memory:read_c_str except that it returns a Vec<u16>. That should use read_scalar to repeatedly read u16 until it finds a NULL, and push everything into a Vec, and return that. (We can't take a shortcut here and directly access the underlying bytes rhe way read_c_str does because we have to care about endianess.)

All that business about actually interpreting this sequence of u16 should be left to Miri.

[RalfJung]

Hint: before pushing, always do ./x.py check. That command completes fairly quickly, and it should find errors such as the ones you are hitting now.

[RalfJung]

Yeah this looks good. :)

Could you please squash the commits? No need to have 17 commits for this.^^

[RalfJung]

Ah I had missed that force-push, GH doesn't send emails for those so in the future please let me know that this is ready for review again. :)

@bors r+ rollup

[bors]

📌 Commit 05f6482 has been approved by RalfJung

[bors]

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened