Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Started working on a post for reviewing unsafe code #8

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Michael-F-Bryan
Copy link
Owner

No description provided.

@Michael-F-Bryan Michael-F-Bryan changed the title Started working on a post for reviewing unsafe code WIP: Started working on a post for reviewing unsafe code Jan 18, 2020
Comment on lines 201 to 203
This `alloc` module allows code to use `crate::alloc::Box` when boxing things
instead of relying on the `Box` added to the prelude by `std` (which isn't in
the prelude for `#[no_std]` crates).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this isn't the main reason. We could do extern crate alloc unconditionally and use alloc::boxed::Box even in std mode, and get the same effect. But that only works on 1.36+. The more verbose setup is necessary in order for anyhow in std mode to support back to 1.34+.

Comment on lines +1261 to +1263
We're starting off with an immutable reference (`&ErrorImpl<E>`) then borrowing
the `_object` field (immutably) and using pointer casts to get rid of the
`const`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast from *const to *mut is required for using NonNull, whose constructors take *mut but is intended for use with both *const and *mut. There isn't a separate NonNull type dedicated to *const.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently discovered that NonNull<T> implements From<&T> and From<&mut T>. So maybe the overall object_downcast() function could look like this?

unsafe fn object_downcast<E>(e: NonNull<ErrorImpl<()>>, target: TypeId) -> Option<NonNull<()>> where E: 'static,
{
    if TypeId::of::<E>() == target {
        let unerased = e.cast::<ErrorImpl<E>>().as_ref();
        Some(NonNull::from(&unerased._object))
    } else {
        None
    }
}

@dtolnay
Copy link

dtolnay commented Jan 26, 2020

If you feel up to it, could you look into what else is being flagged by cargo miri test? I see the following even after commenting out all the downcast_mut in the test suite:

error: Miri evaluation error: trying to reborrow for Unique, but parent tag <88627> does not have an appropriate item in the borrow stack
   --> /git/anyhow/src/error.rs:523:20
    |
523 |     let unerased = mem::transmute::<Box<ErrorImpl<()>>, Box<ErrorImpl<E>>>(e);
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to reborrow for Unique, but parent tag <88627> does not have an appropriate item in the borrow stack
    |
    = note: inside call to `anyhow::error::object_drop::<anyhow::error::ContextError<i32, anyhow::Error>>` at /git/anyhow/src/error.rs:504:13
    = note: inside call to `anyhow::error::<impl std::ops::Drop for anyhow::Error>::drop` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ptr/mod.rs:174:1
note: inside call to `std::intrinsics::drop_in_place::<anyhow::Error> - shim(Some(anyhow::Error))` at tests/test_chain.rs:17:1
   --> tests/test_chain.rs:17:1
    |
17  | }
    | ^
note: inside call to `test_iter` at tests/test_chain.rs:8:1
   --> tests/test_chain.rs:8:1
    |
8   | / fn test_iter() {
9   | |     let e = error();
10  | |     let mut chain = e.chain();
11  | |     assert_eq!("3", chain.next().unwrap().to_string());
...   |
16  | |     assert!(chain.next_back().is_none());
17  | | }
    | |_^
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
    = note: inside call to `<[closure@tests/test_chain.rs:8:1: 17:2] as std::ops::FnOnce<()>>::call_once - shim` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
    = note: inside call to `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:515:5
    = note: inside call to `test::__rust_begin_short_backtrace::<fn()>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:506:30
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libcore/ops/function.rs:232:5
    = note: inside call to `<[closure@DefId(15:633 ~ test[6578]::run_test[0]::{{closure}}[2]) 0:fn()] as std::ops::FnOnce<()>>::call_once - shim(vtable)` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/liballoc/boxed.rs:1015:9
    = note: inside call to `<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send> as std::ops::FnOnce<()>>::call_once` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:318:9
    = note: inside call to `<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>> as std::ops::FnOnce<()>>::call_once` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:305:40
    = note: inside call to `std::panicking::r#try::do_call::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:281:13
    = note: inside call to `std::panicking::r#try::<(), std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
    = note: inside call to `std::panic::catch_unwind::<std::panic::AssertUnwindSafe<std::boxed::Box<dyn std::ops::FnOnce() + std::marker::Send>>, ()>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:539:18
    = note: inside call to `test::run_test_in_process` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:452:39
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:473:13
    = note: inside call to `test::run_test::run_test_inner` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:503:28
    = note: inside call to `test::run_test` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:286:13
    = note: inside call to `test::run_tests::<[closure@DefId(15:232 ~ test[6578]::console[0]::run_tests_console[0]::{{closure}}[2]) 0:&mut test::console::ConsoleTestState, 1:&mut std::boxed::Box<dyn test::formatters::OutputFormatter>]>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/console.rs:282:5
    = note: inside call to `test::run_tests_console` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:122:15
    = note: inside call to `test::test_main` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libtest/lib.rs:141:5
    = note: inside call to `test::test_main_static`
    = note: inside call to `main` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:34
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:73
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/sys_common/backtrace.rs:129:5
    = note: inside call to `std::sys_common::backtrace::__rust_begin_short_backtrace::<[closure@DefId(1:6019 ~ std[d5d1]::rt[0]::lang_start_internal[0]::{{closure}}[0]::{{closure}}[0]) 0:&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:52:13
    = note: inside call to closure at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:305:40
    = note: inside call to `std::panicking::r#try::do_call::<[closure@DefId(1:6018 ~ std[d5d1]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:281:13
    = note: inside call to `std::panicking::r#try::<i32, [closure@DefId(1:6018 ~ std[d5d1]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe]>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394:14
    = note: inside call to `std::panic::catch_unwind::<[closure@DefId(1:6018 ~ std[d5d1]::rt[0]::lang_start_internal[0]::{{closure}}[0]) 0:&&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe], i32>` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:51:25
    = note: inside call to `std::rt::lang_start_internal` at /home/david/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/rt.rs:67:5
    = note: inside call to `std::rt::lang_start::<()>`

@Michael-F-Bryan Michael-F-Bryan force-pushed the reviewing-unsafe-code branch 2 times, most recently from 3d89a6f to 87dce10 Compare January 27, 2020 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants