rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.64k stars 12.49k forks source link

ICE in async closure that takes ref to boxed dyn trait #128554

Closed Almale02 closed 4 weeks ago

Almale02 commented 1 month ago

Code

    let thread_rx: flume::Receiver<(Uuid, Box<dyn ResourceLoader>)> =
        runtime.render_resource_manager.thread_rx.clone();
    let thread_tx: flume::Sender<(Uuid, Box<dyn Any + Send>)> =
        runtime.render_resource_manager.thread_tx.clone();
    std::thread::spawn(move || {
        let rt = tokio::runtime::Builder::new_current_thread()
            .enable_all()
            .build()
            .unwrap();
        let rx = thread_rx;
        let tx = thread_tx;

        rt.spawn(async move {
            let futures = FuturesUnordered::new();
            loop {
                let tx = tx.clone();
                match rx.recv_async().await {
                    // loader has type of Box<dyn ResourceLoader>
                    Ok((uuid, mut loader)) => futures.push(async move || {
                        // this line is causing the problem! I am using async_trait so that also could be a problem
                        let result: Box<dyn Any + Send> = loader.start_loading().await;

                        tx.send((uuid, result)).unwrap();
                    }),
                    Err(_) => continue,
                }
            }
        });
    });

Meta

rustc --version --verbose:

rustc 1.82.0-nightly (8e86c9567 2024-08-01)
binary: rustc
commit-hash: 8e86c9567154dc5a9ada15ab196d23eae2bd7d89
commit-date: 2024-08-01
host: x86_64-pc-windows-msvc
release: 1.82.0-nightly
LLVM version: 19.1.0

Error output

error: internal compiler error: compiler\rustc_middle\src\mir\tcx.rs:84:21: deref projection of non-dereferenceable ty PlaceTy { ty: dyn [Binder { value: Trait(rendering::render_resources::ResourceLoader), bound_vars: [] }] + '{erased}, variant_index: None }

thread 'rustc' panicked at compiler\rustc_middle\src\mir\tcx.rs:84:21:
Box<dyn Any>
Backtrace

``` stack backtrace: 0: 0x7ffd09aa4be4 - std::backtrace_rs::backtrace::dbghelp64::trace at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\..\..\backtrace\src\backtrace\dbghelp64.rs:91 1: 0x7ffd09aa4be4 - std::backtrace_rs::backtrace::trace_unsynchronized at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\..\..\backtrace\src\backtrace\mod.rs:66 2: 0x7ffd09aa4be4 - std::sys::backtrace::_print_fmt at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\sys\backtrace.rs:66 3: 0x7ffd09aa4be4 - std::sys::backtrace::impl$0::print::impl$0::fmt at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\sys\backtrace.rs:39 4: 0x7ffd09ad7049 - core::fmt::rt::Argument::fmt at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\core\src\fmt\rt.rs:173 5: 0x7ffd09ad7049 - core::fmt::write at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\core\src\fmt\mod.rs:1178 6: 0x7ffd09a9ad27 - std::io::Write::write_fmt at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\io\mod.rs:1823 7: 0x7ffd09aa7d09 - std::panicking::default_hook::closure$1 at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\panicking.rs:266 8: 0x7ffd09aa788c - std::panicking::default_hook at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\panicking.rs:293 9: 0x7ffcc09bfd2e - memchr 10: 0x7ffd09aa871b - alloc::boxed::impl$50::call at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\alloc\src\boxed.rs:2162 11: 0x7ffd09aa871b - std::panicking::rust_panic_with_hook at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\panicking.rs:805 12: 0x7ffcc2098cc3 - ::print_variant 13: 0x7ffcc208a6a9 - ::print_variant 14: 0x7ffcc208a659 - ::print_variant 15: 0x7ffcc20a49c5 - ::emit_producing_guarantee 16: 0x7ffcc1fb0f92 - rustc_middle[e966a2a8e22d7b51]::util::bug::bug_fmt 17: 0x7ffcc1f90f3d - rustc_middle[e966a2a8e22d7b51]::ty::consts::const_param_default 18: 0x7ffcc1f90d76 - rustc_middle[e966a2a8e22d7b51]::ty::consts::const_param_default 19: 0x7ffcc1fb0e92 - rustc_middle[e966a2a8e22d7b51]::util::bug::bug_fmt 20: 0x7ffcc07c1dcd - ::projection_ty 21: 0x7ffcbf8f2b0a - ::run_pass 22: 0x7ffcbf8eb414 - ::run_pass 23: 0x7ffcbf850d50 - ::run_pass 24: 0x7ffcbf850af0 - ::run_pass 25: 0x7ffcbf9171df - rustc_mir_transform[f0390e7c3912931f]::optimized_mir 26: 0x7ffcbffd4280 - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 27: 0x7ffcbfebb79c - rustc_ty_utils[e8eaf8771b960b53]::ty::self_ty_of_trait_impl_enabling_order_dep_trait_object_hack 28: 0x7ffcbffdae15 - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 29: 0x7ffcc086c35d - ::encode_def_id 30: 0x7ffcc1fb0b00 - ::coroutine_layout 31: 0x7ffcbfe563db - rustc_ty_utils[e8eaf8771b960b53]::instance::resolve_instance_raw 32: 0x7ffcbfe6d02e - rustc_ty_utils[e8eaf8771b960b53]::layout::layout_of 33: 0x7ffcbffd60a3 - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 34: 0x7ffcbfed7d5b - rustc_ty_utils[e8eaf8771b960b53]::ty::self_ty_of_trait_impl_enabling_order_dep_trait_object_hack 35: 0x7ffcbffe4bdc - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 36: 0x7ffcbf892b74 - ::run_lint 37: 0x7ffcbf851542 - ::run_pass 38: 0x7ffcbf91470f - rustc_mir_transform[f0390e7c3912931f]::mir_drops_elaborated_and_const_checked 39: 0x7ffcbffd5bcb - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 40: 0x7ffcbff13bfc - rustc_ty_utils[e8eaf8771b960b53]::ty::self_ty_of_trait_impl_enabling_order_dep_trait_object_hack 41: 0x7ffcbffdaae3 - rustc_query_impl[5107b6b60d76bada]::plumbing::query_key_hash_verify_all 42: 0x7ffcbf4846ae - rustc_interface[450cdcfa57d2d7e7]::passes::analysis 43: 0x7ffcbf061b3b - ::write_str 44: 0x7ffcbef7cba7 - rustc_ty_utils[e8eaf8771b960b53]::ty::adt_sized_constraint 45: 0x7ffcbf067447 - rustc_query_impl[5107b6b60d76bada]::query_system 46: 0x7ffcbc4e38b9 - __ImageBase 47: 0x7ffcbc4df27f - __ImageBase 48: 0x7ffcbc4ecf5b - __ImageBase 49: 0x7ffd09ab9acd - alloc::boxed::impl$48::call_once at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\alloc\src\boxed.rs:2148 50: 0x7ffd09ab9acd - alloc::boxed::impl$48::call_once at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\alloc\src\boxed.rs:2148 51: 0x7ffd09ab9acd - std::sys::pal::windows::thread::impl$0::new::thread_start at /rustc/8e86c9567154dc5a9ada15ab196d23eae2bd7d89/library\std\src\sys\pal\windows\thread.rs:55 52: 0x7ffd82c8257d - BaseThreadInitThunk 53: 0x7ffd83a2af28 - RtlUserThreadStart ```

this was at the end of the error

query stack during panic:
#0 [optimized_mir] optimizing MIR for `run::{closure#0}::{closure#0}::{closure#0}::{closure#0}::{closure#0}`
#1 [layout_of] computing layout of `{async closure body@krajc\src/main.rs:225:74: 230:22}`
#2 [mir_drops_elaborated_and_const_checked] elaborating drops for `run::{closure#0}::{closure#0}::{closure#0}::{closure#0}`
#3 [analysis] running analysis passes on this crate
end of query stack

i have put a comment in the code where the problematic line is, the problem could be caused by async trait object method in ResourceLoader::start_loading
i am using async_trait crate
here is the library code that the problematic code uses
the ResourceLoader trait is at the end

use std::{
    any::Any,
    collections::{HashMap, HashSet},
    marker::PhantomData,
    path::Path,
    pin::Pin,
    task::{Context, Poll},
};

use async_trait::async_trait;
use futures::{future::BoxFuture, stream::FuturesUnordered, Future, FutureExt};
use tokio::fs;
use uuid::Uuid;

use crate::typed_addr::dupe;

pub struct RenderResourceManager {
    pub resources: HashMap<Uuid, Box<dyn Any + Send>>,
    pub main_tx: flume::Sender<(Uuid, Box<dyn ResourceLoader>)>,
    pub thread_rx: flume::Receiver<(Uuid, Box<dyn ResourceLoader>)>,
    pub main_rx: flume::Receiver<(Uuid, Box<dyn Any + Send>)>,
    pub thread_tx: flume::Sender<(Uuid, Box<dyn Any + Send>)>,
}

pub struct ResourceHandle<T> {
    uuid: Uuid,
    manager: &'static mut RenderResourceManager,
    _p: PhantomData<T>,
}

impl<T> ResourceHandle<T> {
    pub fn new(uuid: Uuid, manager: &'static mut RenderResourceManager) -> Self {
        Self {
            uuid,
            manager,
            _p: PhantomData,
        }
    }
    pub fn is_loaded(&self) -> bool {
        self.manager.resources.contains_key(&self.uuid)
    }
    pub fn get(&self) -> &'static T {
        dupe(self)
            .manager
            .resources
            .get(&self.uuid)
            .unwrap()
            .downcast_ref()
            .unwrap()
    }
    pub fn get_checked(&self) -> Option<&'static T> {
        match self.is_loaded() {
            true => Some(self.get()),
            false => None,
        }
    }

    pub fn get_mut(&self) -> &'static mut T {
        dupe(self)
            .manager
            .resources
            .get_mut(&self.uuid)
            .unwrap()
            .downcast_mut()
            .unwrap()
    }
    pub fn get_mut_checked(&self) -> Option<&'static mut T> {
        match self.is_loaded() {
            true => Some(self.get_mut()),
            false => None,
        }
    }
}

impl RenderResourceManager {
    pub fn new() -> Self {
        let (main_tx, thread_rx) = flume::unbounded();
        let (thread_tx, main_rx) = flume::unbounded();
        Self {
            resources: HashMap::default(),
            main_tx,
            thread_rx,
            main_rx,
            thread_tx,
        }
    }

    pub fn load_resource<T>(&mut self, loader: Box<dyn ResourceLoader>) -> ResourceHandle<T> {
        let uuid = Uuid::new_v4();

        self.main_tx.send((uuid, loader)).unwrap();

        ResourceHandle::new(uuid, dupe(self))
    }
}

impl Default for RenderResourceManager {
    fn default() -> Self {
        Self::new()
    }
}

#[async_trait]
pub trait ResourceLoader: Send {
    async fn start_loading(&mut self) -> Box<dyn Any + Send>;
}

pub struct FileResourceLoader<T: FileLoadable + 'static> {
    pub path: String,
    _p: PhantomData<T>,
}

#[async_trait]
impl<T: FileLoadable + Send + 'static> ResourceLoader for FileResourceLoader<T> {
    async fn start_loading(&mut self) -> Box<dyn Any + Send> {
        let path = self.path.clone();
        let bytes = fs::read(path).await;

2852.txt)
        let mut load_provider = T::default();
        Box::new(load_provider.process_bytes(bytes)) as Box<dyn Any + Send>
    }
}

pub trait FileLoadable: Default {
    type FinalResource: Send;
    fn process_bytes(&mut self, file: std::io::Result<Vec<u8>>) -> Self::FinalResource;
}

impl<T> Unpin for FileResourceLoader<T> where T: FileLoadable + Send + 'static {}

[rustc-ice-2024-08-02T15_03_52-2852.txt](https://github.com/user-attachments/files/16472328/rustc-ice-2024-08-02T15_03_52-

Almale02 commented 1 month ago

update on the issue

i changed ResourceLoader::start_loading from using async_trait to manually returning BoxFuture<'static, Box<dyn Any + Send>> and it still errors

here is the updated code

pub trait ResourceLoader: Send {
    fn start_loading(&mut self) -> BoxFuture<'static, Box<dyn Any + Send>>;
}

pub struct FileResourceLoader<T: FileLoadable + 'static> {
    pub path: String,
    _p: PhantomData<T>,
}

impl<T: FileLoadable + Send + 'static> ResourceLoader for FileResourceLoader<T> {
    fn start_loading(&mut self) -> BoxFuture<'static, Box<dyn Any + Send>> {
        let path = self.path.clone();

        async move {
            let bytes = fs::read(path).await;
            let mut load_provider = T::default();

            Box::new(load_provider.process_bytes(bytes)) as Box<dyn Any + Send>
        }
        .boxed()
    }
}
Almale02 commented 1 month ago

good news! i got my code to compile, here is the final verson

    let thread_rx: flume::Receiver<(Uuid, Box<dyn ResourceLoader>)> =
        runtime.render_resource_manager.thread_rx.clone();
    let thread_tx: flume::Sender<(Uuid, Box<dyn Any + Send>)> =
        runtime.render_resource_manager.thread_tx.clone();
    std::thread::spawn(move || {
        let rt = tokio::runtime::Builder::new_current_thread()
            .enable_all()
            .build()
            .unwrap();
        let rx = thread_rx;
        let tx = thread_tx;

        rt.spawn(async move {
            let futures = FuturesUnordered::new();
            loop {
                let tx = tx.clone();

                match rx.recv() {
                    Ok((uuid, mut loader)) => {
                        let future = async move {
                            let result: Box<dyn Any + Send> = loader.start_loading().await;

                            tx.send((uuid, result)).unwrap();
                        }
                        .boxed();
                        futures.push(future);
                    }
                    Err(_) => continue,
                }
            }
        });
    });

i dont now for sure what was the problem but i have an idea and i think the problem was with the futures variable type representation

at the start i was pushing futures to the "futures" variable that had in the types signiture "impl", when i tried to manually write out the type of the "futures" variable

 let futures: FuturesUnordered<impl FnMut() -> impl Future<Output = ()>> =FuturesUnordered::new();

i couldnt because rust didnt allow the use of "impl" here, cargo check was throwing errors, when i realised this, i tried to find a type for T generic in FuturesUnordered than didnt had this problem, and i now have this

  let futures: FuturesUnordered<std::pin::Pin<Box<dyn Future<Output = ()> + Send>>> = FuturesUnordered::new(); 

and the ICE error disapperad

i think the reason why the compiler was crashing is because internally it couldnt use the impl in the generic type signiture and it crashed

but i could be totally wrong, i have never worked on the rust compiler

i wont close this issue because the error is still in the compiler and it probably worth a fix

compiler-errors commented 1 month ago

You should probably provide a fully working reproduction for this crash. Specifically, you shared a lot of code but it's still missing whatever dupe function is being called all over the codebase lol

Almale02 commented 1 month ago

You should probably provide a fully working reproduction for this crash. Specifically, you shared a lot of code but it's still missing whatever dupe function is being called all over the codebase lol

yeah dupe is a little bit sketchy and i probably shouldnt it basically transforms any lifetime into 'static i know that it is unsafe and all of that but i only use it to return references from EngineRuntime(it stores all state of my game engine) which is leaked and 'static i didnt have any issues from it so far

here is the repo https://github.com/Almale02/krajc

i will try to post the minimal reproduction to it later, probably tomorrow

here is the dupe

pub fn dupe<T: ?Sized>(value: &T) -> &'static mut T {
    unsafe { &mut *((value as *const T) as *mut T) }
}
compiler-errors commented 1 month ago

for the record, that is super duper hyper major unsound!

but it shouldn't crash the compiler

Almale02 commented 1 month ago

for the record, that is super duper hyper major unsound!

but it shouldn't crash the compiler

yeah "dupe" definitely not the problem here i used it so much without any problems that i could put my life on it

compiler-errors commented 1 month ago

Well, could you please still provide a full reproduction? I don't have any idea how to actually reproduce this ICE with the code you've shared.

Almale02 commented 1 month ago

yeah i will try to do it, probably tomorrow

btw are you talking about the minimal reproduction of the error?

compiler-errors commented 1 month ago

No, I just mean something that's complete. You've shared a bunch of random snippets of code and I have no idea how to put them together in context to make something that actually compiles, let alone ICEs 😃

compiler-errors commented 1 month ago

Minimal reproduction:

#![feature(async_closure)]

use std::pin::Pin;
use std::future::Future;

pub trait ResourceLoader {
    fn start_loading(&mut self);
}

fn run(mut loader: Box<dyn ResourceLoader>) {
    async move || {
        loader.start_loading();
    };
}
Almale02 commented 1 month ago

the reproduction looks correct, i dont know if you need that FuturesUnordered for it probably my theory why it wasnt working is bullshit :)

i am pretty new to this github stuff i am currenlty figuring out things

i will also try to do some kind of reproduction myself

compiler-errors commented 1 month ago

This is just a bug in async closures. I don't need a reproduction anymore; I doubt you'll get one that's smaller than the one I pased above.

Almale02 commented 1 month ago

that was the problem? i didnt even realize that async closures were experimental

well, thanks for figuring out what was the problem with it!

so the problem basically was that you cannot call trait object methods inside async move closures? i mean i was doing the same thing in my new code that works but the reproduction that you posted does exactly that and it crashes so probably i dont understand something , but atleast it is resolved

Almale02 commented 1 month ago

also it is strange that cargo check works but build doesnt it probably doesnt actually compile the code it just runs static analysis on it or something

glad i could help with this issue!

lqd commented 1 month ago

I doubt you'll get one that's smaller than the one I pased above

just remove the two unused imports to get a smaller repro 🤡

workingjubilee commented 1 month ago

@Almale02 While it is secondary to this issue, if fn dupe is sound, could you explain how was I able to write a program that used nothing but your function and safe code, yet the value of the integer held by &mut... exclusive reference, which should forbid all mutation except directly via that reference... has changed?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b2c7095a0a56a7326cf6d591b51546f6

Almale02 commented 1 month ago

@Almale02 While it is secondary to this issue, if fn dupe is sound, could you explain how was I able to write a program that used nothing but your function and safe code, yet the value of the integer held by &mut... exclusive reference, which should forbid all mutation except directly via that reference... has changed?

play.rust-lang.org?version=stable&mode=debug&edition=2021&gist=b2c7095a0a56a7326cf6d591b51546f6

yes fn dupe is completely unsafe and if incorrectly used it could lead to ub, like the code you provided in the playground, like if you do this

    let v = vec![1000, 2000, 3000];
    let dup = dupe(&v[0]);
    println!("{}", v[0]);
    println!("{}", dup);
    drop(v);
    //let v = vec![9001; 9001];
    *dup = 3; // HERE!
    println!("{}", dup);

you could modify freed values, because the dup variable has 'static lifetime and it is not bounded at all to the v vector so it could cause problems even if you use it in a single threaded environment, i didnt thought about this issue, and i never encountered it, mainly because i only use the dupe to either return 'static references from Box::leak-ed EngineRuntime which is completelty safe because it wont be ever dropped, or like in this case

        dupe(&physics).physics_pipeline.step(
            &gravity.0,
            &IntegrationParameters::default(),
            &mut dupe(&physics).island_manager,
            &mut dupe(&physics).broad_phase,
            &mut dupe(&physics).narrow_phase,
            &mut dupe(&physics).rigid_body_set,
            &mut dupe(&physics).collider_set,
            &mut dupe(&physics).impulse_joint_set,
            &mut dupe(&physics).multibody_joint_set,
            &mut dupe(&physics).ccd_solver,
            Some(&mut dupe(&physics).query_pipeline),
            &event_handler,
            &event_handler,
        );

i need to use pass down multiple &mut references, then i also use it, technically there could be problems with it, but so far i didnt encounter any problems with it because i use dupe correctly

i have plans to migrate my project to use proper lifetimes, but i couldnt get the rendering code to work with lifetimes because it was throwing bunch of lifetime errors that i couldnt solve

so yes, dupe is completely unsafe and bypasses the compiler but i know how to use it, and i didnt meant other people to use it, but eventially i will have to figure out how to use lifetimes

i never stated that dupe is sound, it obviously isnt, but it is sound to me because i only use it when it is sound to use and your example definitely wasnt :)

workingjubilee commented 1 month ago

@Almale02 Ah! You should mark that function as unsafe, then!