Implement futex_wait and futex_wake. #1568

m-ou-se · 2020-10-01T18:50:53Z

Fixes rust-lang/rust#77406 and fixes #1562.

This makes std's park(), park_timeout(), and unpark() work. That means std::sync::Once is usable again and the test pass again with the latest rustc.

This also makes parking_lot work.

m-ou-se · 2020-10-01T18:53:13Z

Before today I was only vaguely aware of the existence of miri and never touched it, so please check if I'm not doing anything weird in this code. ^^'

This does not support futex_wait with a timeout yet.

src/shims/posix/linux/foreign_items.rs

src/sync.rs

src/shims/posix/linux/foreign_items.rs

futex_wake doesn't access the futex itself, so should accept pointers to memory that's no longer there.

src/sync.rs

src/shims/posix/linux/sync.rs

m-ou-se · 2020-10-02T08:51:27Z

Updated! Handling the addr pointer better now and added comments.

RalfJung

We're getting there. :) In fact modulo comments I'd be fine landing this, though it does not yet entirely match the semantics you describe. But we can leave FIXMEs and do that later, whatever you prefer.

It would be good to have a test as well that directly probes the API -- but again we can leave that to the future, for now libstd itself serves as a test.

src/shims/posix/linux/sync.rs

RalfJung · 2020-10-02T18:10:37Z

src/shims/posix/linux/sync.rs

+    // The first three arguments (after the syscall number itself) are the same to all futex operations:
+    //     (int *addr, int op, int val).
+    // Although note that the first one is often passed as a different pointer type, e.g. `*const AtomicU32` or `*mut u32`.
+    let addr = this.deref_operand(args[1])?;


Wait, didn't you say that this point may in fact be dangling? That won't work like this. deref_operand requires that the pointer be dereferncable for the given type.

I am okay leaving this as a FIXME though. But eventually we should have a test for this.

Wait, didn't you say that this point may in fact be dangling?

I did! I (mis)interpreted your comment above ..

Then later you can use this.read_scalar(futex_val_place.offset) to read the actual data, and futex_val_place.ptr.assert_ptr() to get the Pointer. (deref_operand does the necessary int-to-ptr conversion.)

.. as that only read_scalar() requires the address to be readable, and that deref_operand only does the conversion to a pointer.

Anyway, this was the reason I kept the original addr around as a TyOp, separate from the Pointer, such that the FUTEX_WAIT path can still call deref_operand (or read_scalar_at_offset, which already does deref_operand) on it.

What's a good way to do this? Both operations need addr as Pointer, but one also needs to dereference the pointer (as i32).

I'm using force_ptr now instead of deref_operand. But using that Pointer to read the i32 pointee in FUTEX_WAIT seems tricky.

Now I'm using check_ptr_access followed by this.memory.get_raw(addr.alloc_id)?.read_scalar, but I don't know if that's a proper way to do it.

Yes this is a tricky situation which I do not think we've had before. But get_raw feels like going down way too far the stack of abstractions.

I think the best way is to delay part of the decision: here at the top, just do let futex_val = this.read_immediate. Then also get the ptr value via force_ptr(futex_val.to_scalar()?). In the futex_wait branch, do deref_operand(futex_val.into())?. At least I think that should work.

That only works if I do:

this.read_scalar(this.deref_operand(addr.into())?.offset(Size::ZERO, MemPlaceMeta::None, this.machine.layouts.i32, this)?.into())?.to_i32()?

or

this.read_scalar_at_offset(addr.into(), 0, this.machine.layouts.i32)?.to_i32()?

in the futex_wait branch. Otherwise it refuses to read a AtomicI32 as i32 again.

Hm, now it fails if somebody calls syscall() with that futex address as a usize. Neither parking_lot nor std do that, but it might make sense as in Rust *const i32 is not Send, while usize is. And since this pointer may dangle, sending it to another thread might make sense.

I am confused actually, why does this fail? Without deref_operand I think this should work, but I might forget some check somewhere.

Or was the actual problem that you passed 0usize? In that case the value would have been the problem, not the type -- i.e., 0usize as *const () would not work either. That is what the last comment that I added is about.

I am confused actually, why does this fail?

Because in the example I also used the usize for the wait operation, which does dereference it.

But it doesn't do that with deref_operand. So as long as the pointer points to something valid, with your current code, I think it should work fine both with usize and with a pointer type.

Oh, read_scalar_at_offset uses deref_operand internally...

src/shims/posix/linux/sync.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

m-ou-se · 2020-10-02T22:18:33Z

Added support FUTEX_WAIT with a timeout, and a test for park/park_timeout/unpark.

Many of parking_lots tests now pass:

MIRIFLAGS=-Zmiri-disable-isolation cargo miri test -- --skip condvar --skip recursive

The condvar and recursive tests fail an assertion.

Backtrace:

thread 'rustc' panicked at 'assertion failed: `(left == right)`
  left: `8`,
 right: `4`', /rust/compiler/rustc_middle/src/mir/interpret/value.rs:401:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/154f1f544dd68f7b53ff8d9952811e855f4c2d7c/library/std/src/panicking.rs:483
   1: std::panicking::begin_panic_fmt
             at /rustc/154f1f544dd68f7b53ff8d9952811e855f4c2d7c/library/std/src/panicking.rs:437
   2: rustc_middle::mir::interpret::value::Scalar<Tag>::to_bits_or_ptr
   3: rustc_mir::interpret::memory::Memory<M>::force_bits
             at /rust/compiler/rustc_mir/src/interpret/memory.rs:1027
   4: rustc_mir::interpret::eval_context::InterpCx<M>::force_bits
             at /rust/compiler/rustc_mir/src/interpret/eval_context.rs:353
   5: rustc_mir::interpret::operator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::overflowing_binary_op
             at /rust/compiler/rustc_mir/src/interpret/operator.rs:322
   6: rustc_mir::interpret::operator::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::binop_ignore_overflow
             at /rust/compiler/rustc_mir/src/interpret/operator.rs:42
   7: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_rvalue_into_place
             at /rust/compiler/rustc_mir/src/interpret/step.rs:175
   8: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::statement
             at /rust/compiler/rustc_mir/src/interpret/step.rs:89
   9: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::step
             at /rust/compiler/rustc_mir/src/interpret/step.rs:65
  10: miri::eval::eval_main::{{closure}}
             at /rust/src/tools/miri/src/eval.rs:221
  11: miri::eval::eval_main
             at /rust/src/tools/miri/src/eval.rs:215

Edit: Ah. I returned an int (i32) from syscall(), but that should've been a long (isize).

m-ou-se · 2020-10-02T22:38:01Z

All tests of parking_lot now pass, except for:

test_rwlock_recursive
This test 'leaks' a thread: It spawns a thread that doesn't get joined before the end of the main thread. Changing the test to unblock and join the thread at the end makes it pass.
test_condvar_requeue
This test 'deadlocks' because Miri never switches to the other thread, as its scheduler is not preemptive. Adding a thread::yield_now() makes it pass.
All webkit_queue_test tests
These don't seem to finish. They do make constant progress, but these stress tests are just extremely slow on Miri. Reducing messages_per_producer to 100 or 10 makes them all pass.

Many tests do result in Miri reporting a memory leak, but that's probably just a consequence of how parking_lot works.

src/shims/posix/linux/sync.rs

tests/run-pass/concurrency/parking.rs

RalfJung · 2020-10-03T09:36:27Z

All tests of parking_lot now pass, except for:

That's really cool. :)

Ah. I returned an int (i32) from syscall(), but that should've been a long (isize).

So looks like this is not checked by the libstd user then?
Before resolving the issue (but probably in a separate PR to unblock fixing Miri) we should have a test that directly uses the futex API to check this. It should also use a dangling addr to make sure that works as well.

src/shims/posix/linux/sync.rs

Otherwise `FUTEX_WAIT with timeout` will look weird.

m-ou-se · 2020-10-03T09:41:06Z

So looks like this is not checked by the libstd user then?

Yup, std::thread::park_timeout() doesn't return why it woke up, so it doesn't care what the syscall returns. In parking_lot, park() and unpark() also don't check the return value (in release mode), but park_timeout() does return whether it woke up due to a timeout or not, so does check the return value and errno.

we should have a test that directly uses the futex API

Yes, definitely.

m-ou-se · 2020-10-03T11:09:44Z

we should have a test that directly uses the futex API

Added a test.

src/shims/posix/linux/sync.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

src/shims/posix/linux/sync.rs

Co-authored-by: Ralf Jung <post@ralfj.de>

RalfJung · 2020-10-03T12:35:58Z

Thanks a lot. :)

@bors r+

bors · 2020-10-03T12:36:00Z

📌 Commit 68776d2 has been approved by RalfJung

bors · 2020-10-03T12:36:08Z

⌛ Testing commit 68776d2 with merge b0e02f0...

bors · 2020-10-03T12:59:59Z

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

src/helpers.rs

m-ou-se mentioned this pull request Oct 1, 2020

miri no longer builds after rust-lang/rust#76919 rust-lang/rust#77406

Closed

m-ou-se changed the title ~~Implement futex_wait and futex_wait.~~ Implement futex_wait and futex_wake. Oct 1, 2020

Implement futex_wait and futex_wake.

1ffc5bb

This does not support futex_wait with a timeout yet.

m-ou-se force-pushed the futex branch from 4fa5376 to 1ffc5bb Compare October 1, 2020 18:56