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

add C API for epoch_deadline_callback and wasmtime_error_t creation #6359

Merged

Conversation

theothergraham
Copy link
Contributor

issue #6277

This adds C API interfaces to:

  1. Set the epoch deadline callback to a C function
  2. Create a wasmtime_error_t from C

@theothergraham theothergraham requested a review from a team as a code owner May 9, 2023 14:13
@theothergraham theothergraham requested review from fitzgen and removed request for a team May 9, 2023 14:13
@theothergraham theothergraham changed the title add C API for epoch_dead_callback and wasmtime_error_t creation add C API for epoch_deadline_callback and wasmtime_error_t creation May 9, 2023
@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label May 9, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, thanks!

Comment on lines 27 to 30
Some(Box::new(wasmtime_error_t::from(anyhow!("{:?}", unsafe {
std::ffi::CStr::from_ptr(msg)
}))))
}
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of :? this should use .to_str() on the pointer received to avoid Rust-level string escaping which shows up in the debug representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.to_str() leaves me with the potential to get a Utf8Error instead of a &str. I was not sure what the preferred way to deal with such bad input would be, but based on what I see in crates/c-api/src/trap.rs, I think I will go with String::from_utf8_lossy(). Sound good?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think the _lossy variant is fine. That's a reality of working with cross-language FFI here but in the context of "just an error message" the lossy variant should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, revised.

@alexcrichton alexcrichton added this pull request to the merge queue May 9, 2023
Merged via the queue into bytecodealliance:main with commit cfb506b May 9, 2023
@kpreisser
Copy link
Contributor

kpreisser commented May 10, 2023

Hi! From my testing, it seems the new wasmtime_error_new function in the C API doesn't copy the string passed by the const char* argument when being called; instead the supplied char pointer is dereferenced later after wasmtime_error_new has already returned, e.g. when returning the wasmtime_error_t* in the epoch deadline callback. Is this intended?

I would have expected a similar behavior as wasmtime_trap_new, which copies the string when being called, so that the memory holding the string can be freed by the caller after the function returns. Otherwise, it's not quite clear to me when the memory can be freed.

Thanks!

@alexcrichton
Copy link
Member

Ah thanks for testing! That's a mistake in the API and a subtle detail with the implementation. It's definitely not intended that the pointer should remain live, the contents should be copied into the error.

The line that needs to change is this one, namely the anyhow!(msg_string) should become anyhow!("{msg_string}"). @theothergraham or @kpreisser would one of y'all be up for a PR along those lines?

@theothergraham
Copy link
Contributor Author

🤦 Sorry, that's my Rust newbieness showing. I missed that String:: from_utf8_lossy() only makes a copy if it finds invalid uft-8. I'll make a PR. Thank you for catching that, @kpreisser!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants