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

component-model: bindgen macro generates traits whose lifetimes are too loose for state with explicit lifetime #7088

Closed
lwansbrough opened this issue Sep 26, 2023 · 9 comments

Comments

@lwansbrough
Copy link
Contributor

lwansbrough commented Sep 26, 2023

Consider the example from the bindgen docs:

use wasmtime::component::*;
use wasmtime::{Config, Engine, Store};
use my::project::host::Host;

bindgen!();

struct MyState {
    // ...
}

// Note that the trait here is per-interface and within a submodule now.
impl Host for MyState {
    fn gen_random_integer(&mut self) -> wasmtime::Result<u32> {
        Ok(rand::thread_rng().gen())
    }

    fn sha256(&mut self, bytes: Vec<u8>) -> wasmtime::Result<String> {
        // ...
    }
}

fn main() -> wasmtime::Result<()> {
    let mut config = Config::new();
    config.wasm_component_model(true);
    let engine = Engine::new(&config)?;
    let component = Component::from_file(&engine, "./your-component.wasm")?;

    let mut linker = Linker::new(&engine);
    HelloWorld::add_to_linker(&mut linker, |state: &mut MyState| state)?;

    let mut store = Store::new(
        &engine,
        MyState { /* ... */ },
    );
    let (bindings, _) = HelloWorld::instantiate(&mut store, &component, &linker)?;

    // Note that the `demo` method returns a `&Demo` through which we can
    // run the methods on that interface.
    bindings.demo().call_run(&mut store)?;
    Ok(())
}

If MyState here is instead MyState<'render> (in my case, MyState owns an Option<wgpu::RenderPass<'render>>), the bindgen macro generates a trait which is implemented like this:

impl<'render> HostGpuCommandEncoder for WizardRuntimeState<'render> {
    fn begin_render_pass<'life0, 'async_trait>(&'life0 mut self, self_: wasmtime::component::Resource<GpuCommandEncoder>, descriptor: GpuRenderPassDescriptor) ->  ::core::pin::Pin<Box<dyn ::core::future::Future<Output = wasmtime::Result<wasmtime::component::Resource<GpuRenderPassEncoder>>> + ::core::marker::Send+'async_trait>> where 'life0: 'async_trait, Self:'async_trait {

but here, Rust is unable to assume that 'life0 outlives 'render and I cannot specify it as 'life0: 'render because that's a stricter requirement than the trait.

@alexcrichton
Copy link
Member

alexcrichton commented Sep 26, 2023

Could you share some more code as a reproducible example? This for example compiles ok for me locally:

mod with_lifetimes {

    wasmtime::component::bindgen!({
        inline: "
package my:project

interface host {
    gen-random-integer: func() -> u32
    sha256: func(bytes: list<u8>) -> string
}

world hello-world {
    import host

    export demo: interface {
        run: func()
    }
}
        ",
        async: true,
    });

    struct MyState<'a> {
        _x: Option<wgpu::RenderPass<'a>>,
    }

    #[async_trait::async_trait]
    impl my::project::host::Host for MyState<'_> {
        async fn gen_random_integer(&mut self) -> wasmtime::Result<u32> {
            loop {}
        }

        async fn sha256(&mut self, _bytes: Vec<u8>) -> wasmtime::Result<String> {
            loop {}
        }
    }
}

@lwansbrough
Copy link
Contributor Author

@alexcrichton apologies for the increase in complexity of the example, but this more accurately reflects the problem I'm having and demonstrates the correct lifetimes issue. Uses slab and wgpu

mod with_lifetimes {

    wasmtime::component::bindgen!({
        inline: "
package my:project

interface host {
    gen-random-integer: func() -> u32
    sha256: func(bytes: list<u8>) -> string
}

world hello-world {
    import host

    export demo: interface {
        run: func()
    }
}
        ",
        async: true,
    });

    struct MyState<'a> {
        render_state: Option<RenderState<'a>>,
    }

    pub struct RenderState<'a> {
        pub command_encoders: slab::Slab::<GpuCommandEncoderState<'a>>,
    }

    impl<'a> RenderState<'a> {
        pub fn new() -> RenderState<'a> {
            RenderState {
                command_encoders: slab::Slab::new()
            }
        }
    }

    pub struct GpuCommandEncoderState<'a> {
        pub encoder: wgpu::CommandEncoder,
        pub render_passes: slab::Slab::<wgpu::RenderPass<'a>>
    }

    impl<'a> GpuCommandEncoderState<'a> {
        pub fn new(encoder: wgpu::CommandEncoder) -> GpuCommandEncoderState<'a> {
            GpuCommandEncoderState {
                encoder,
                render_passes: slab::Slab::new()
            }
        }
    }

    #[async_trait::async_trait]
    impl my::project::host::Host for MyState<'_> {
        async fn gen_random_integer(&mut self) -> wasmtime::Result<u32> {
            let render_state = self.render_state.as_mut().expect("Render state");
            let command_encoder_state = render_state.command_encoders.get_mut(0 as usize).unwrap();
            
            let render_pass_encoder = command_encoder_state.encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
                label: None,
                color_attachments: &[],
                depth_stencil_attachment: None
            });
            
            let pass_id = command_encoder_state.render_passes.insert(render_pass_encoder);

            wasmtime::Result::Ok(1)
        }

        async fn sha256(&mut self, _bytes: Vec<u8>) -> wasmtime::Result<String> {
            loop {}
        }
    }
}

@alexcrichton
Copy link
Member

Thanks! I unfortunately think though that this isn't related to Wasmtime or bindgen!. The error message from rustc could probably be improved in this case, but for example this fails to compile which has all the wasmtime bits removed:

struct MyState<'a> {
    render_state: Option<RenderState<'a>>,
}

pub struct RenderState<'a> {
    pub command_encoders: slab::Slab<GpuCommandEncoderState<'a>>,
}

impl<'a> RenderState<'a> {
    pub fn new() -> RenderState<'a> {
        RenderState {
            command_encoders: slab::Slab::new(),
        }
    }
}

pub struct GpuCommandEncoderState<'a> {
    pub encoder: wgpu::CommandEncoder,
    pub render_passes: slab::Slab<wgpu::RenderPass<'a>>,
}

impl<'a> GpuCommandEncoderState<'a> {
    pub fn new(encoder: wgpu::CommandEncoder) -> GpuCommandEncoderState<'a> {
        GpuCommandEncoderState {
            encoder,
            render_passes: slab::Slab::new(),
        }
    }
}

impl MyState<'_> {
    fn gen_random_integer(&mut self) {
        let render_state = self.render_state.as_mut().expect("Render state");
        let command_encoder_state = render_state.command_encoders.get_mut(0 as usize).unwrap();

        let render_pass_encoder =
            command_encoder_state
                .encoder
                .begin_render_pass(&wgpu::RenderPassDescriptor {
                    label: None,
                    color_attachments: &[],
                    depth_stencil_attachment: None,
                });

        command_encoder_state
            .render_passes
            .insert(render_pass_encoder);

        loop {}
    }
}

I believe the root cause of this is that wgpu::RenderPass<'a> is storing a pointer to the wgpu::CommandEncoder field, which makes your GpuCommandEncoderState type a self-referential structure, which Rust doesn't handle well.

That's at least the problem, but I don't know enough about wgpu to know how best to restructure to fix this. Regardless though I think that this is unrelated to Wasmtime?

@lwansbrough
Copy link
Contributor Author

Ok, that's a good assessment. I'll have to do some more digging on my end to figure out how to manage that. I'm happy to close the issue in that case. Thanks for your help!

@lwansbrough
Copy link
Contributor Author

I guess I do have one follow up @alexcrichton -- specifying that the lifetime of self ('life0, defined by the async_trait) is longer than 'render fixes the problem, which I think is how I need to define the lifetime relationship (the global state lifetime outlives the lifetime of the render call.) If you remove the wasmtime trait, this code is valid. But wasmtime doesn't allow it because there is no way to specify 'life0: 'render. There may be an alternative way to configure this struct but as it stands, wasmtime is indeed getting in the way here.

impl<'life0: 'render, 'render> MyState<'render> {
    async fn gen_random_integer(&'life0 mut self) -> wasmtime::Result<u32> {
        let render_state = self.render_state.as_mut().expect("Render state");
        let command_encoder_state = render_state.command_encoders.get_mut(0 as usize).unwrap();
        
        let render_pass_encoder = command_encoder_state.encoder.begin_render_pass(&wgpu::RenderPassDescriptor {
            label: None,
            color_attachments: &[],
            depth_stencil_attachment: None
        });
        
        let pass_id = command_encoder_state.render_passes.insert(render_pass_encoder);

        wasmtime::Result::Ok(1)
    }

    async fn sha256(&mut self, _bytes: Vec<u8>) -> wasmtime::Result<String> {
        loop {}
    }
}

@lwansbrough lwansbrough reopened this Sep 26, 2023
@alexcrichton
Copy link
Member

Unfortunately while that works and passes the compiler it is not possible to actually call that function. 'life0: 'render can be thought of "'life0 outlives 'render". In your object first though 'render must exist first because that's used to create MyState. Afterwards MyState is then borrowed with 'life0 which is guaranteed to be shorter than 'render. That ends up meaning that even if you could write that down it would be impossible to safely call.

@lwansbrough
Copy link
Contributor Author

😕 I think I'd rather deal with use after free bugs... Thank you for the explanation and I understand that this out of scope for wasmtime so I appreciate you taking the time to make that clear to me.

@alexcrichton
Copy link
Member

If you're up for it I suspect the wgpu authors would be happy to hear from you and might be able to assist. You may not be the first that wanted to store those two structures next to each other so they may have pointers about how to approach this already.

@lwansbrough
Copy link
Contributor Author

lwansbrough commented Sep 26, 2023

I'll give it a try, thanks again for your help. Edit: yeah... gfx-rs/wgpu#1453

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

No branches or pull requests

3 participants
@alexcrichton @lwansbrough and others