Add shims for RwLock::try_read/RwLock::try_write

[divergentdave]

This implements proper locking so that we can check for reentrancy and implement the try_* methods.

Fixes #781

[RalfJung]

This was already attempted in #905, and the same concerns as there apply: I don't think it is a good idea to land this as is.

(Also, you are swamping us with the sheer amount of PRs. Your contributions are appreciated, but it might take a few weeks a week or two until I get through all of them. I still have a backlog of >150 Rust notifications from just coming back from a week of absence.)

[RalfJung]

That said, once I am through that backlog, I'd be happy to mentor you for fixing #781 if you are interested :)

[divergentdave]

Sure, totally understood. I will read over those and work on re-implementing these shims.

[divergentdave]

I think I need some pointers on working with memory in Miri to get this implemented properly. Mutex/lock behavior when locked twice from the same thread depends on the attributes/kind of the synchronization primitive, so we need to know the kind of the mutex. The hard part is that this has to interoperate with PTHREAD_MUTEX_INITIALIZER etc. from glibc. In the libc crate, PTHREAD_MUTEX_INITIALIZER is a const of type pthread_mutex_t, which ultimately is a struct containing a zeroed [u8] array of the appropriate size. In C, PTHREAD_MUTEX_INITIALIZER is a struct literal with more fields. There are other preprocessor macros that put different values in the mutex's "kind" field, and their libc crate equivalents, such as PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP, put the same byte patterns in memory using array literals.

I tried using write_immediate_to_mplace naively with an i32 and an mplace made from an offset pointer, but this failed with an "invalid Scalar layout" error. I assume that since pthread_mutex_t is defined as a struct holding a byte array, I can't use this function with the "wrong" type and layout of a single i32.

What would be the best way to read and write fields in these quasi-opaque FFI structs? The two vague ideas I have at this point are using this.memory.write_bytes() (though this would require whatever the equivalent of i32::to_ne_bytes is for the interpreted platform), or perhaps there's another low-level API that allows some degree of type punning.

[RalfJung]

Mutex/lock behavior when locked twice from the same thread depends on the attributes/kind of the synchronization primitive, so we need to know the kind of the mutex.

I imagined we'd just ignore that and always treat reentrant locking as an error.

I tried using write_immediate_to_mplace naively with an i32 and an mplace made from an offset pointer, but this failed with an "invalid Scalar layout" error. I assume that since pthread_mutex_t is defined as a struct holding a byte array, I can't use this function with the "wrong" type and layout of a single i32.

I am not entirely sure what it is that you did, but the error probably arises because you used write_scalar or so and the type of the place does not fit to the scalar you used. Memory in Rust/Miri is untyped, but memory accesses are typed, so you have to make sure the place and the thing you are putting in the place match.

But also, your description is so high-level that I have little clue what concretely it is you are trying to do, so I can't really make any suggestions for how to do it.

[divergentdave]

I got it working on Linux, but I need to revisit my memory layout on macOS, because mutex and rwlock objects have a 32-byte signature at the beginning of their static initializer.

Edit: fixed!

[RalfJung]

Before reviewing this huge PR, it'd help if you could answer my questions above -- I don't see yet why we should even care about how exactly these platforms represent what. Why can't we just make all locks error on reentrancy?

[divergentdave]

The standard library uses both NORMAL and RECURSIVE mutexes, via std::sys_common::mutex::Mutex (cf. std::io::lazy::Lazy) and std::sys_common::remutex::ReentrantMutex (cf. std::io::stdio::Stdout). These differ in behavior enough that it shows up in a single threaded environment. If a thread calls lock twice in a row on a NORMAL mutex, it should deadlock, and if you do the same on a RECURSIVE mutex, it bumps a counter, succeeds both times, and the thread is supposed to later unlock it twice in a row.

[RalfJung]

FWIW I'd be perfectly fine to make RECURSIVE not properly supported (i.e., to error when they are taken recursively).

But now you already put in all the work. ;) It might just take a bit until I get around to a review, there are at least 3 other PRs before it in my queue.

[divergentdave]

I just added a separate test that exercises PTHREAD_MUTEX_RECURSIVE, by calling println!() from within a println!(). If I copy-paste the PTHREAD_MUTEX_NORMAL behavior where RECURSIVE is in the shims, this test case fails with the deadlock error message.

[RalfJung]

I just added a separate test that exercises PTHREAD_MUTEX_RECURSIVE, by calling println!() from within a println!(). If I copy-paste the PTHREAD_MUTEX_NORMAL behavior where RECURSIVE is in the shims, this test case fails with the deadlock error message.

Nice catch! I didn't even know that relied in reentrant mutexes.

It would also be good to have a test that exercises the locking API more directly, as we have little control over how e.g. println! locking will change in the future. You already do some of that in sync.rs; also doing it for PTHREAD_MUTEX_RECURSIVE would be great.

[divergentdave]

Okay, I added a test for that. I had to do it via libc, because sys_common/sys are private, and ReentrantMutex isn't exposed or used anywhere else.

[bors]

☔ The latest upstream changes (presumably #1167) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

Let's see how these new tests do on Windows... @bors try

[RalfJung]

Do you think it would make sense to switch this to cfg(target_os = "linux") as well? Would be good to be consistent within the file here.

[RalfJung]

Aside from that last nit, this is ready to go. :)

[divergentdave]

Nit addressed!

[RalfJung]

Thanks a lot, this is such a great new feature. :)
@bors r+

[bors]

📌 Commit 80497e5 has been approved by RalfJung

[bors]

⌛ Testing commit 80497e5 with merge d935f62...

[bors]

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