Implement epoll shim #3712
Implement epoll shim #3712
Conversation
|
@rustbot author |
rustbot
added
the
S-waiting-on-author
|
Maybe you need to rebase |
|
80d407e is an attempt to pass in
I am not sure if it is possible to make it work, but I am going to revert this an try to pass ICE error messagethread 'rustc' panicked at src/shims/unix/fd.rs:236:45:
already borrowed: BorrowMutError
stack backtrace:
0: 0x74cf3217ddd5 - std::backtrace_rs::backtrace::libunwind::trace::he59335385543b4e7
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5
1: 0x74cf3217ddd5 - std::backtrace_rs::backtrace::trace_unsynchronized::h7c84e90889d9a1bb
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
2: 0x74cf3217ddd5 - std::sys::backtrace::_print_fmt::h4d4fd356397c3378
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:66:9
3: 0x74cf3217ddd5 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::hde110f186e471219
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:39:26
4: 0x74cf321cecfb - core::fmt::rt::Argument::fmt::h08e1f010bbba14d0
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/fmt/rt.rs:173:76
5: 0x74cf321cecfb - core::fmt::write::hb225859127e7765b
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/fmt/mod.rs:1178:21
6: 0x74cf3217261f - std::io::Write::write_fmt::h210b8145a2e5358b
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/io/mod.rs:1823:15
7: 0x74cf32180671 - std::sys::backtrace::BacktraceLock::print::h2cba555a5ecfba6e
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:42:9
8: 0x74cf32180671 - std::panicking::default_hook::{{closure}}::h908da943d0764776
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:266:22
9: 0x74cf3218034c - std::panicking::default_hook::h78057f076c8a8ad8
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:293:9
10: 0x74cf35644af9 - std[47b6bda5310033f4]::panicking::update_hook::<alloc[8f32396a97214ba5]::boxed::Box<rustc_driver_impl[bad6c552c0296adc]::install_ice_hook::{closure#0}>>::{closure#0}
11: 0x74cf3218103f - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::h0f07d18a9e7ba2b2
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2162:9
12: 0x74cf3218103f - std::panicking::rust_panic_with_hook::h8cf4d1eff41758c5
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:805:13
13: 0x74cf32180c67 - std::panicking::begin_panic_handler::{{closure}}::hc381813fafeb3969
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:671:13
14: 0x74cf3217e299 - std::sys::backtrace::__rust_end_short_backtrace::h688ac9f52e29476a
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/backtrace.rs:170:18
15: 0x74cf321808f4 - rust_begin_unwind
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:662:5
16: 0x74cf321cb2c3 - core::panicking::panic_fmt::hf0000f31cd37d561
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/panicking.rs:74:14
17: 0x74cf321c8103 - core::cell::panic_already_borrowed::hce2c3af5b8e750d8
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/cell.rs:787:5
18: 0x61f340c419d0 - core::cell::RefCell<T>::borrow_mut::hc8491909e95877c9
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/cell.rs:1078:25
19: 0x61f340c419d0 - miri::shims::unix::fd::FileDescriptionRef::borrow_mut::ha6b56c880ba131fa
at /home/byt/Documents/miri/src/shims/unix/fd.rs:236:21
20: 0x61f340c419d0 - miri::shims::unix::linux::epoll::EvalContextExt::check_and_update_readiness::hb3bfc5ed26545b74
at /home/byt/Documents/miri/src/shims/unix/linux/epoll.rs:449:38
21: 0x61f340ce34db - <miri::shims::unix::linux::eventfd::Event as miri::shims::unix::fd::FileDescription>::read::h95373e54e5d409b5
at /home/byt/Documents/miri/src/shims/unix/linux/eventfd.rs:97:13
22: 0x61f340c2f14e - miri::shims::unix::fd::EvalContextExt::read::h6387a12f7f75f421
at /home/byt/Documents/miri/src/shims/unix/fd.rs:576:21
23: 0x61f340c2481d - miri::shims::unix::foreign_items::EvalContextExt::emulate_foreign_item_inner::hdf8f4f8957eade9c
at /home/byt/Documents/miri/src/shims/unix/foreign_items.rs:95:30
24: 0x61f340c6f090 - miri::shims::foreign_items::EvalContextExtPriv::emulate_foreign_item_inner::hf33eb5da0886c6d4
at /home/byt/Documents/miri/src/shims/foreign_items.rs:952:25
25: 0x61f340c6b84a - miri::shims::foreign_items::EvalContextExt::emulate_foreign_item::hb3d83275619b51de
at /home/byt/Documents/miri/src/shims/foreign_items.rs:70:15
26: 0x61f340bbd4b5 - <miri::machine::MiriMachine as rustc_const_eval::interpret::machine::Machine>::find_mir_or_eval_fn::h12f89890355ac47f
at /home/byt/Documents/miri/src/machine.rs:984:20
27: 0x61f340bbd4b5 - rustc_const_eval::interpret::terminator::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::eval_fn_call::h5139575d80e9aa6a
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:621:46
28: 0x61f340bfb821 - rustc_const_eval::interpret::terminator::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::eval_terminator::h282a1bcffca2f0eb
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/terminator.rs:133:17
29: 0x61f340bfb821 - rustc_const_eval::interpret::step::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::terminator::hc66a0ed88e387be6
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/step.rs:383:9
30: 0x61f340bfb821 - rustc_const_eval::interpret::step::<impl rustc_const_eval::interpret::eval_context::InterpCx<M>>::step::h71c560bbe4128a31
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_const_eval/src/interpret/step.rs:51:9
31: 0x61f340bfb821 - miri::concurrency::thread::EvalContextExt::run_threads::h4b8d54facb86cd82
at /home/byt/Documents/miri/src/concurrency/thread.rs:1197:25
32: 0x61f340b1917e - miri::eval::eval_entry::{{closure}}::h0a868308d84278ae
at /home/byt/Documents/miri/src/eval.rs:445:49
33: 0x61f340b1917e - core::ops::function::FnOnce::call_once::h8458d316094a605b
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/ops/function.rs:250:5
34: 0x61f340b1917e - <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once::h6dcd64c25e38427b
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/core/src/panic/unwind_safe.rs:272:9
35: 0x61f340b1917e - std::panicking::try::do_call::h2a8bc7bd42ad739a
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:554:40
36: 0x61f340b1917e - std::panicking::try::h3e09c3f57cb7121b
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panicking.rs:518:19
37: 0x61f340b1917e - std::panic::catch_unwind::hd082b45a3f557876
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/panic.rs:345:14
38: 0x61f340b1917e - miri::eval::eval_entry::h9606b7c20aa35ad7
at /home/byt/Documents/miri/src/eval.rs:445:9
39: 0x61f340a8e349 - <miri::MiriCompilerCalls as rustc_driver_impl::Callbacks>::after_analysis::{{closure}}::h4ed3975824bb13a7
at /home/byt/Documents/miri/src/bin/miri.rs:114:40
40: 0x61f340a8e349 - rustc_middle::ty::context::GlobalCtxt::enter::{{closure}}::h993e552016cccdcd
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context.rs:1331:37
41: 0x61f340a8e349 - rustc_middle::ty::context::tls::enter_context::{{closure}}::h520a6e8d4a69a77c
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context/tls.rs:82:9
42: 0x61f340a8e349 - std::thread::local::LocalKey<T>::try_with::h821dca7f769b72a2
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/thread/local.rs:283:12
43: 0x61f340a8e349 - std::thread::local::LocalKey<T>::with::h577858e84b543777
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/thread/local.rs:260:9
44: 0x61f340a8e349 - rustc_middle::ty::context::tls::enter_context::h359a4877bd951015
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context/tls.rs:79:9
45: 0x61f340a8e349 - rustc_middle::ty::context::GlobalCtxt::enter::h9aabebea09eac648
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_middle/src/ty/context.rs:1331:9
46: 0x61f340a92b19 - rustc_interface::queries::QueryResult<&rustc_middle::ty::context::GlobalCtxt>::enter::h6667d8429b7307c4
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/compiler/rustc_interface/src/queries.rs:65:9
47: 0x61f340a92b19 - <miri::MiriCompilerCalls as rustc_driver_impl::Callbacks>::after_analysis::h4dd7be76087912d2
at /home/byt/Documents/miri/src/bin/miri.rs:74:9
48: 0x74cf379f5dce - rustc_interface[2879afccd290b137]::interface::run_compiler::<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}
49: 0x74cf379db709 - std[47b6bda5310033f4]::sys::backtrace::__rust_begin_short_backtrace::<rustc_interface[2879afccd290b137]::util::run_in_thread_with_globals<rustc_interface[2879afccd290b137]::util::run_in_thread_pool_with_globals<rustc_interface[2879afccd290b137]::interface::run_compiler<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>
50: 0x74cf379db4ba - <<std[47b6bda5310033f4]::thread::Builder>::spawn_unchecked_<rustc_interface[2879afccd290b137]::util::run_in_thread_with_globals<rustc_interface[2879afccd290b137]::util::run_in_thread_pool_with_globals<rustc_interface[2879afccd290b137]::interface::run_compiler<core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>, rustc_driver_impl[bad6c552c0296adc]::run_compiler::{closure#0}>::{closure#1}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#0}::{closure#0}, core[88f6a1db45837a48]::result::Result<(), rustc_span[29f3c8e11ce55b6c]::ErrorGuaranteed>>::{closure#1} as core[88f6a1db45837a48]::ops::function::FnOnce<()>>::call_once::{shim:vtable#0}
51: 0x74cf3218afbb - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::hc2643f81a499fd0c
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2148:9
52: 0x74cf3218afbb - <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once::ha8d56f3a1fc33872
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/alloc/src/boxed.rs:2148:9
53: 0x74cf3218afbb - std::sys::pal::unix::thread::Thread::new::thread_start::h9d7a26a6515c8c04
at /rustc/dba8e2d2c2890a8b9e88cbf4855ac5711337946c/library/std/src/sys/pal/unix/thread.rs:105:17
54: 0x74cf31e94ac3 - start_thread
at ./nptl/pthread_create.c:442:8
55: 0x74cf31f26850 - __GI___clone3
at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
56: 0x0 - <unknown>
error: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/miri/issues/new
note: please make sure that you have updated to the latest nightly
note: please attach the file at `/home/byt/Documents/miri/rustc-ice-2024-08-09T07_29_32-2219326.txt` to your bug report
query stack during panic:
end of query stack
Miri caused an ICE during evaluation. Here's the interpreter backtrace at the time of the panic:
note: the place in the program where the ICE was triggered
--> ./tests/pass-dep/libc/libc-epoll.rs:478:29
|
478 | let res: i32 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8).try_into().unwrap() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: BACKTRACE:
= note: inside `test_event_overwrite` at ./tests/pass-dep/libc/libc-epoll.rs:478:29: 478:71
note: inside `main`
--> ./tests/pass-dep/libc/libc-epoll.rs:8:5
|
8 | test_event_overwrite();
| ^^^^^^^^^^^^^^^^^^^^^^
= note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
= note: inside `std::sys::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/backtrace.rs:154:18: 154:21
= note: inside closure at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:164:18: 164:75
= note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/core/src/ops/function.rs:284:13: 284:31
= note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:554:40: 554:43
= note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:518:19: 518:88
= note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panic.rs:345:14: 345:33
= note: inside closure at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:143:48: 143:73
= note: inside `std::panicking::r#try::do_call::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:554:40: 554:43
= note: inside `std::panicking::r#try::<isize, {closure@std::rt::lang_start_internal::{closure#2}}>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:518:19: 518:88
= note: inside `std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panic.rs:345:14: 345:33
= note: inside `std::rt::lang_start_internal` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:143:20: 143:98
= note: inside `std::rt::lang_start::<()>` at /home/byt/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/rt.rs:163:17: 168:6
error: test failed, to rerun pass `--test ui` |
src/shims/unix/linux/eventfd.rs
Outdated
| @@ -153,7 +150,7 @@ impl FileDescription for Event { | |||
| // When any of the event happened, we check and update the status of all supported event | |||
| // types for current file description. | |||
| use crate::shims::unix::linux::epoll::EvalContextExt; | |||
| ecx.check_and_update_readiness(fd_ref)?; | |||
| ecx.check_and_update_readiness(self.id, || self.get_epoll_ready_events())?; | |||
There was a problem hiding this comment.
So here it is actually crucial that we access self.get_epoll_ready_events and do not get a second borrow of the RefCell for this FD... well that is tricky.
Please add a comment here explaining that. Also, please add a comment next to the version of check_and_update_readiness that takes a separate ID and closure and explain that this is an internal helper and should not usually be used.
I wonder if we should just say that we always call check_and_update_readiness on the FD that was read or written after each read or write... but that also feels like a hack. Or we could have the id inside the FD type for each FD, but that also gets annoying. This seems to be one of these rare cases where inheritance would actually be nice, since we could have a "base class" with the id field and each FD type just inherits that.
|
All the comments were addressed, this is ready for another round of review. ^^ |
|
4812fa6 ensures that notification will be provided if Related discussion is in https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/epoll.20notification.20on.20socketpair.20write.20unblock |
|
Do we really want to emulate this behavior? I think epoll totally permits us to wake up these threads even when there is just one byte of space available for writing. If we do, I think we shouldn't use "empty" as the threshold. More like, if at least 1KiB can be written, or at least half the buffer can be written, or something like that. I wonder what threshold the real implementation uses, but I doubt it is "empty" since that risks starving the readers -- the goal here is presumably to keep a good balance between readers and writers, so "half the buffer" seems like the most naive option to me. This should come with a comment indicating that we do this to avoid waking up threads when they could only write a few bytes. |
I actually prefer to not emulate this behavior and instead provide epoll notification every time |
|
I am completely fine with waking threads up immediately when there's just one byte writeable, even if the real implementation behaves differently for optimization reasons. There should be a comment in the code explaining that. Not sure if @oli-obk has a preference either way. |
|
I too prefer just notifying whenever anything becomes writable/readable, irrespective of the size. |
|
Okay updated, thanks for helping figure this out! |
|
|
|
☀️ Test successful - checks-actions |
| } | ||
|
|
||
| // When read/write happened on one side of the socketpair, only the other side will be notified. | ||
| fn test_epoll_socketpair_special_case() { |
There was a problem hiding this comment.
Why is this a "special case"?
When an FD changes status, only that FD gets notified. That's completely normal.
There was a problem hiding this comment.
Oh this is a test case added when I realised that socketpair fd shouldn't notify itself, it should notify it's peer instead. This behaviour is different from eventfd . I think the name is fixed in your PR, thanks for that!
There was a problem hiding this comment.
The behavior is the same as eventfd. It notifies the FD that changed status. Of course you have to check properly for each case which FDs change status. :)
| )?; | ||
| num_of_events = num_of_events.checked_add(1).unwrap(); | ||
| } else { | ||
| break; |
There was a problem hiding this comment.
Isn't this wrong? This means that we popped an event from the ready list, but we didn't store it to the output array. It just gets discarded entirely.
When fixing this, please make sure to also add a test for this case.
There was a problem hiding this comment.
Yup, thanks for catching this, it is currently fixed in #3818
This PR:
epollfor Implement basic epoll support #3448 . The design for this PR is documented in https://hackmd.io/@tiif/SJatftrH0 .