Getpid shim

[b-ncMN]

No description provided.

[b-ncMN]

I was not able to test and verify if the windows function work properly, this happens when I try to :

cargo miri run --target x86_64-pc-windows-msvc
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `/home/infrandomness/.cargo/bin/cargo-miri target/miri/x86_64-pc-windows-msvc/debug/getpidtest.exe`
warning: unnecessary `unsafe` block
  --> src/main.rs:12:5
   |
12 |     unsafe {
   |     ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

error: unsupported operation: can't call foreign function: GetCurrentProcessId
  --> src/main.rs:5:19
   |
5  |         let pid = processthreadsapi::GetCurrentProcessId();
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't call foreign function: GetCurrentProcessId
   |
   = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
           
           ```
           
The code I am using is : 
```rs
use winapi::um::processthreadsapi;

extern "C" fn foo() {
    unsafe {
        let pid = processthreadsapi::GetCurrentProcessId();
        println!("pid : {}", pid);

    }
}

fn main() {
    unsafe {
        foo();
    }
}

I tried giving multiple different ABIs to foo without success, I can't seem to be able to call winapi::..::GetCurrentProcessId()

[RalfJung]

This is a good start, thanks. :) I left some comments.

What this is also missing is a test. The test should probably just call std::process::id.

[b-ncMN]

This seems ready to me, is there anything else?

[RalfJung]

Please also add a test that runs on all platforms and just calls std::process::id(), to make sure that does not error. Currently, the Windows codepath is uncovered by the test suite.

[b-ncMN]

Please also add a test that runs on all platforms and just calls std::process::id(), to make sure that does not error. Currently, the Windows codepath is uncovered by the test suite.

I couldn't find where to create windows' test, libc.rs seems to be reserved for unixy OSs

[b-ncMN]

Please also add a test that runs on all platforms and just calls std::process::id(), to make sure that does not error. Currently, the Windows codepath is uncovered by the test suite.

I couldn't find where to create windows' test, libc.rs seems to be reserved for unixy OSs

Also winapi does not seem to be a dependency of miri, so I wonder if I'm supposed to create the test into test/pass/....rs

[RalfJung]

I couldn't find where to create windows' test, libc.rs seems to be reserved for unixy OSs

Just create a file that calls std::process::id(). That will work on all platforms. This is not a libc test.

[b-ncMN]

I couldn't find where to create windows' test, libc.rs seems to be reserved for unixy OSs

Just create a file that calls std::process::id(). That will work on all platforms. This is not a libc test.

This is weird, I'm having this as an error while trying what you told me :

actual output differed from expected tests/pass/getpid.stderr
Diff < left / right > :
>error: unsupported operation: `getpid` not available when isolation is enabled
>  --> RUSTLIB/std/src/sys/PLATFORM/os.rs:LL:CC
>   |
>LL |     unsafe { libc::getpid() as u32 }
>   |              ^^^^^^^^^^^^^^ `getpid` not available when isolation is enabled
>   |
>   = help: pass the flag `-Zmiri-disable-isolation` to disable isolation;
>   = help: or pass `-Zmiri-isolation-error=warn` to configure Miri to return an error code from isolated operations (if supported for that operation) and continue with a warning
>           
>   = note: inside `std::sys::PLATFORM::os::getpid` at RUSTLIB/std/src/sys/PLATFORM/os.rs:LL:CC
>   = note: inside `std::process::id` at RUSTLIB/std/src/process.rs:LL:CC
>note: inside `getpid` at $DIR/getpid.rs:LL:CC
>  --> $DIR/getpid.rs:LL:CC
>   |
>LL |     std::process::id()
>   |     ^^^^^^^^^^^^^^^^^^
>note: inside `main` at $DIR/getpid.rs:LL:CC
>  --> $DIR/getpid.rs:LL:CC
>   |
>LL |     getpid();
>   |     ^^^^^^^^
>
>note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
>
>error: aborting due to previous error
>
>



There were 1 unmatched diagnostics that occurred outside the testfile and had not pattern
    Error: unsupported operation: `getpid` not available when isolation is enabled

failures:
    tests/pass/getpid.rs

meanwhile my code is :

// compile-flags: -Zmiri-isolation-error=warn-nobacktrace

fn getpid() -> u32 {
    std::process::id()
}

fn main() {
    getpid();
}

Oh right, std::process::id() calls std::sys::unix::os::getpid(), which itself calls libc::getpid(), therefore, what in the world

[RalfJung]

The flag needs to be -Zmiri-disable-isolation.

-Zmiri-isolation-error only works for shims that can return an error code when isolation is enabled. Which getpid isn't.

[b-ncMN]

I've tried to cleanup a bit the mess that this tree is, let me know if you need me to squash further

[RalfJung]

This LGTM but please squash it all into a single commit. The last commit doesn't even change anything.^^

[RalfJung]

Oh, that failure is funny. It only occurs with optimizations enabled.

Does inlining here "work around" the this.frame_in_std() check? That would be a problem...

[RalfJung]

Yeah this is the full error:

error: unsupported operation: can't call foreign function: GetCurrentProcessId
 --> tests/pass/getpid.rs:4:5
  |
4 |     std::process::id()
  |     ^^^^^^^^^^^^^^^^^^ can't call foreign function: GetCurrentProcessId
  |
  = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
          
  = note: inside `getpid` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/windows/os.rs:327:14
note: inside `main` at tests/pass/getpid.rs:8:5
 --> tests/pass/getpid.rs:8:5
  |
8 |     getpid();
  |     ^^^^^^^^

Notice the "inside getpid at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/windows/os.rs:327:14" -- getpid is our function but the span is in the stdlib!

I guess so far we have been lucky that MIR inlining didn't do this for other calls.

Can we check the current span instead, somehow? That should be stable under inlining (as the backtrace shows).

[b-ncMN]

Yeah this is the full error:

error: unsupported operation: can't call foreign function: GetCurrentProcessId
 --> tests/pass/getpid.rs:4:5
  |
4 |     std::process::id()
  |     ^^^^^^^^^^^^^^^^^^ can't call foreign function: GetCurrentProcessId
  |
  = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
          
  = note: inside `getpid` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/windows/os.rs:327:14
note: inside `main` at tests/pass/getpid.rs:8:5
 --> tests/pass/getpid.rs:8:5
  |
8 |     getpid();
  |     ^^^^^^^^

Notice the "inside getpid at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/windows/os.rs:327:14" -- getpid is our function but the span is in the stdlib!

I guess so far we have been lucky that MIR inlining didn't do this for other calls.

Can we check the current span instead, somehow? That should be stable under inlining (as the backtrace shows).

The issue didn't reproduce in the latest CI jobs,
this is odd

[RalfJung]

The issue didn't reproduce in the latest CI jobs,
this is odd

It only reproduced on windows. And I fixed it in #2225. :)

[RalfJung]

Thanks!
@bors r+

[bors]

📌 Commit 3e03054 has been approved by RalfJung

[b-ncMN]

The issue didn't reproduce in the latest CI jobs,
this is odd

It only reproduced on windows. And I fixed it in #2225. :)

Oh, I thought I had to apply that fix in here somewhere,
amazing,
so glad this finally merged

[bors]

⌛ Testing commit 3e03054 with merge c5f7a7d...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing c5f7a7d to master...