bytecodealliance / lucet

Lucet, the Sandboxing WebAssembly Compiler.
Apache License 2.0
4.06k stars 164 forks source link

include type of Provided termination details for debugging #630

Closed iximeow closed 3 years ago

iximeow commented 3 years ago

an embedder of lucet will plausibly want to take some action on the Provided details if and when a hostcall hits lucet_hostcall_terminate!. to do this, they would have to downcast to a known type. if, through a bug, lucet_hostcall_terminate! is called with an unknown type, embedders can't get even the name of that type to begin debugging!

at the point you have a Box<dyn Any>, i can't find a way to get a type name anyway. thankfully, the real type is available at the point a user calls TerminationDetails::provide (through lucet_hostcall_terminate! probably. hopefully!), so recording the type at that point is an API-preserving change. except that TerminationDetails itself changes. :grimacing:

awortman-fastly commented 3 years ago

rebased on #631, so this can pass the cargo audit step. fixed a few straggler cases of Provided() which this makes Provided {}.

benaubin commented 3 years ago

@iximeow You should just be able to call type_name() on the Box<dyn Any>. Rust automatically provides the type name.

iximeow commented 3 years ago

@benaubin what would the type parameter be? all i could coax out of type_name after-Anying was a dyn Any:

use std::any::Any;

pub fn type_name_of_val_but_not_unstable<T: ?Sized>(_: &T) -> &'static str {
    std::any::type_name::<T>()
}

pub fn main() {
    let boxed: Box<dyn Any> = Box::new(1);
    println!("{}", type_name_of_val_but_not_unstable(&boxed));
    println!("{}", std::any::type_name::<dyn Any>());
}

->

alloc::boxed::Box<dyn core::any::Any>
dyn core::any::Any
benaubin commented 3 years ago

Oh, yeah, just tried this in the Rust playground. Can't get it to give anything other than Any either 😞. I still really don't love the idea of passing through the type name as a string, though. But yeah, looks like there's no other way around it. Do you think it would be possible to do this in debug only?

iximeow commented 3 years ago

it turns out this was something of a happy accident, in fact. i don't think this should be debug-only because either the field simply doesn't exist in release builds (API's that change between build configs :fearful:) or the field is Option<&'static str> or just "" in release builds (at which point, why?)

there isn't a performance overhead for grabbing the type name since the caller gets monomorphized on the name that it'll pick anyway. so the biggest issue i see is that you can reflect on the type name and do really gnarly stuff.

this is the happy accident: i'd meant to make the type name accessible outside the crate to print when we see an unexpected type somewhere. reading your comment, i realized i didn't add any helper for that. but Provided now prints the type of whatever got provided, in its Debug impl! so it never needed to be accessible outside the crate in the first place.

what has you wanting type names not present in release builds?

benaubin commented 3 years ago

@pchickey Oh no! I really hope not: as an embedder, being able to provide our own termination details with dyn Any is extremely useful - we have an enum of custom reasons an instance may terminate, and it's especially useful to be able to send that back to the host context. By always terminating with the same enum, we've avoided nearly all debugging problems related to the dyn Any. Without a generic on Instance, there's no other way for us to pass custom data.

iximeow commented 3 years ago

once more - had a merge conflict with #632.

@benaubin i think to-Any-or-not-to-Any will end up being a conversation elsewhere (as much as i don't love the dyn Any, i also don't particularly want to change it :grin:). i would point out that unless you're not using wiggle/wasi at all, i suspect you do have at least two possible types of values there though: your enum and whatever gets provided to lucet_hostcall_terminate! in proc_exit. that was an i32 or a String, but #632 makes that wiggle::Trap, leaving specific handling left up to users. so if something could possibly proc_exit in the webassembly guest, :skull: !

benaubin commented 3 years ago

Makes sense. We are not using wasi.