Add `_umtx_op` shim to freebsd #2321

b-ncMN · 2022-07-03T22:01:01Z

This fixes the concurrent-panic.rs test on freebsd

Signed-off-by: InfRandomness infrandomness@gmail.com

b-ncMN · 2022-07-04T19:14:32Z

Well yeah that's what I've been doing already for the past 2 PRs.

I know, but I just worry that list will get bigger and bigger, and the script isn't really the best place for it. Also if we do something similar for other targets, I think we want to make it easier to make sure we don't regress things, and make it easier to figure out what new things are passing.

I'm not saying anything is wrong here, I'm only trying to figure out if we can improve this. Sorry for hijacking your PR for this ^^ I'll mark it as off topic and take it elsewhere

sure, how could we do that?

oli-obk · 2022-07-05T08:28:54Z

I opened #2324 for that

b-ncMN · 2022-07-05T22:15:41Z

This MR is ready for reviews
it is not currently mergeable because the code is to be improved by a large margin, however, I would like some reviews to help me make it better

src/shims/unix/freebsd/futex.rs

oli-obk · 2022-07-06T07:30:13Z

src/shims/unix/freebsd/futex.rs

+    uaddr: &OpTy<'tcx, Tag>,
+    uaddr2: &OpTy<'tcx, Tag>,


Use the Pointer type for these two and move the conversion to the caller

I couldn't make that work, I've posted about it on zulip

ok, they are unused for now anyway, so let's just ignore them (keep OpTy and we'll see about them once they are used)

oli-obk · 2022-07-06T07:34:11Z

src/shims/unix/freebsd/futex.rs

+                        // The `val` value is invalid. Double check this against the manual.
+                        let einval = this.eval_libc("EINVAL").ok()?;
+                        this.set_last_error(einval).ok()?;
+                        None


This seems wrong. https://man7.org/linux/man-pages/man2/futex.2.html suggests that this is not an error case:

This operation tests that the value at the futex word
pointed to by the address uaddr still contains the
expected value val, and if so, then sleeps waiting for a
FUTEX_WAKE operation on the futex word.

If the futex value does not match val, then the call fails
immediately with the error EAGAIN.

https://helpmanual.io/man2/_umtx_op-freebsd/ says

The current value of the variable pointed to by the Fa obj argument is compared with the Fa val . If they are equal, the requesting thread is put to interruptible sleep until woken up or the optionally specified timeout expires.

and does not say what happens if the value differs.

This seems wrong. man7.org/linux/man-pages/man2/futex.2.html suggests that this is not an error case:

This operation tests that the value at the futex word
pointed to by the address uaddr still contains the
expected value val, and if so, then sleeps waiting for a
FUTEX_WAKE operation on the futex word.

If the futex value does not match val, then the call fails
immediately with the error EAGAIN.

You seem to be looking at the linux futex function, not the freebsd _umtx_op function

yes, but the freebsd function does not document what happens if the value differs either. I guess erroring out is the safe thing to do and we can address this if it becomes a problem.

yes, but the freebsd function does not document what happens if the value differs either. I guess erroring out is the safe thing to do and we can address this if it becomes a problem.

we do sort of error out, just with a different code

oli-obk · 2022-07-06T07:43:44Z

src/shims/unix/freebsd/futex.rs

+        Ok(op) =>
+            match op {
+                OpType::UmtxOpWait =>
+                    if this.read_scalar(obj).ok()?.to_u64().ok()? == val {


I think you want to deref_operand first, otherwise you're comparing the pointer address with val

Also I don't know what Pointer to a variable of type Vt long . in the man pages means, could this be a pointer to a 32 bit value on 32 bit platforms?

Also I don't know what Pointer to a variable of type Vt long . in the man pages means, could this be a pointer to a 32 bit value on 32 bit platforms?

I'm not entirely sure, I'll search around

src/shims/unix/freebsd/futex.rs

RalfJung · 2022-07-07T19:32:11Z

ci.sh

@@ -60,7 +60,7 @@ case $HOST_TARGET in
    MIRI_TEST_TARGET=i686-unknown-linux-gnu run_tests
    MIRI_TEST_TARGET=aarch64-apple-darwin run_tests
    MIRI_TEST_TARGET=i686-pc-windows-msvc run_tests
-    MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal hello integer vec current_dir data_race env
+    MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal hello integer vec current_dir data_race env concurrent-panic


If this adds support for futexes, shouldn't there be a lot more tests that work now?
concurrent-panic is an awfully specific test to check. It's not really about concurrency at all. Which part of the Rust stdlib uses _umtx_op that is called by concurrent-panic? AFAIK it uses an RwLock, is that where it comes from?

Answering my own question, it's in condvar.

Specifically I don't think this will give us good test coverage for the new operation, that's why we also want some other tests. At least sync should be added, I think. If those all pass, that gives me higher confidence that the implementation is correct.

RalfJung · 2022-07-07T19:32:48Z

src/shims/unix/freebsd/sync.rs

+    }
+}
+
+struct Futex<'mir, 'tcx> {}


What is the point of this type? Please add a doc comment.

What is the point of this type? Please add a doc comment.

This is just here to be able to bound an implementation

In that case please remove it and follow the extension trait pattern that all the other files are using.

RalfJung · 2022-07-07T19:33:33Z

src/shims/unix/freebsd/sync.rs

+    pub fn futex<'tcx>(
+        this: &mut MiriEvalContext<'_, 'tcx>,
+        // Object to operate on
+        obj: &OpTy<'tcx, Tag>,


All the other arguments are already interpreted at some Rust type; please do the same with this one. The comment seems to indicate it's a pointer, so then this should have type Pointer<Option<Tag>>.

RalfJung · 2022-07-07T19:34:43Z

src/shims/unix/freebsd/sync.rs

+use crate::InterpResult;
+use crate::OpTy;
+use crate::{MiriEvalContext, Tag};
+use std::fmt::Pointer;


You do not want to import this trait here. It's the wrong Pointer.^^

Best to start by copy-pasting the header from some other file. I think you can replace all your imports here by use crate::*; which is what we have in most files.

RalfJung · 2022-07-07T19:36:00Z

src/shims/unix/freebsd/sync.rs

+                            this.futex_wait(
+                                this.read_scalar(obj)?.to_machine_usize()?,
+                                this.get_active_thread(),
+                                u32::MAX,


Suggested change

u32::MAX,

u32::MAX, // we set the bitset to include all bits

b-ncMN · 2022-08-04T20:39:28Z

I am not planning to continue on adding support to freebsd, having lost interest to code in rust (and therefore not wanting to do more on the related rust-lang projects)

RalfJung · 2022-08-04T21:08:40Z

@InfRandomness that's a bummer. Anyway thanks for the work you did; adding new Unix targets is now easier than it used to be! :)

This fixes the `concurrent-panic.rs` test on freebsd Signed-off-by: InfRandomness <infrandomness@gmail.com>

Signed-off-by: InfRandomness <infrandomness@gmail.com>

- Changes return type of futex() to InterpResult<'tcx> and removes every ok() calls - Add documentation and link to `_umtx_op` man pages - Move comments to parameters and remove pointless assignments - Replace `OpTy`s usages to `Pointer`s - Run `deref_operand` on `read_scalar` output in Futex wait operation - Replace `to_u64` to `to_machine_usize` Signed-off-by: InfRandomness <infrandomness@gmail.com>

RalfJung · 2023-02-11T17:22:41Z

There hasn't been any activity here for 4 months, so I am going to close this PR. @InfRandomness or anyone else who is interested, feel free to pick this up again in the future if/when you have time!

b-ncMN force-pushed the _umtx_op-shim branch from ffbabc3 to 4a92f62 Compare July 4, 2022 06:53

This comment was marked as off-topic.

Sign in to view

b-ncMN marked this pull request as ready for review July 5, 2022 22:15

b-ncMN force-pushed the _umtx_op-shim branch 2 times, most recently from f2982b4 to 14cfe9f Compare July 6, 2022 04:45

oli-obk requested changes Jul 6, 2022

View reviewed changes

oli-obk self-assigned this Jul 6, 2022

RalfJung reviewed Jul 7, 2022

View reviewed changes

RalfJung marked this pull request as draft July 9, 2022 14:28

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 11, 2022

b-ncMN closed this Aug 4, 2022

RalfJung mentioned this pull request Aug 4, 2022

Wanted: FreeBSD target maintainer #2462

Closed

b-ncMN reopened this Oct 19, 2022

b-ncMN added 3 commits October 19, 2022 21:44

Add _umtx_op shim to freebsd

195df49

This fixes the `concurrent-panic.rs` test on freebsd Signed-off-by: InfRandomness <infrandomness@gmail.com>

Enable concurrent-panic.rs test in CI

060149e

Signed-off-by: InfRandomness <infrandomness@gmail.com>

b-ncMN force-pushed the _umtx_op-shim branch from 4eacb0f to a92bc63 Compare October 19, 2022 20:02

RalfJung closed this Feb 11, 2023

RalfJung mentioned this pull request May 4, 2024

Implement futex support on FreeBSD (_umtx_op) #3553

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `_umtx_op` shim to freebsd #2321

Add `_umtx_op` shim to freebsd #2321

b-ncMN commented Jul 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

b-ncMN commented Jul 4, 2022

oli-obk commented Jul 5, 2022

b-ncMN commented Jul 5, 2022

oli-obk Jul 6, 2022

b-ncMN Jul 6, 2022 •

edited

Loading

oli-obk Jul 7, 2022

oli-obk Jul 6, 2022

oli-obk Jul 6, 2022

b-ncMN Jul 6, 2022

oli-obk Jul 7, 2022

b-ncMN Jul 7, 2022

oli-obk Jul 6, 2022

oli-obk Jul 6, 2022

b-ncMN Jul 6, 2022

RalfJung Jul 7, 2022

RalfJung Jul 7, 2022

RalfJung Jul 7, 2022

b-ncMN Jul 8, 2022

RalfJung Jul 8, 2022

RalfJung Jul 7, 2022

RalfJung Jul 7, 2022

RalfJung Jul 7, 2022

b-ncMN commented Aug 4, 2022

RalfJung commented Aug 4, 2022

RalfJung commented Feb 11, 2023

Add _umtx_op shim to freebsd #2321

Add _umtx_op shim to freebsd #2321

Conversation

b-ncMN commented Jul 3, 2022

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

b-ncMN commented Jul 4, 2022

oli-obk commented Jul 5, 2022

b-ncMN commented Jul 5, 2022

Choose a reason for hiding this comment

b-ncMN Jul 6, 2022 • edited Loading

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

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

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

b-ncMN commented Aug 4, 2022

RalfJung commented Aug 4, 2022

RalfJung commented Feb 11, 2023

Add `_umtx_op` shim to freebsd #2321

Add `_umtx_op` shim to freebsd #2321

b-ncMN Jul 6, 2022 •

edited

Loading