-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
c-api: add wasmtime_trap_code #3086
c-api: add wasmtime_trap_code #3086
Conversation
Subscribe to Label Actioncc @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:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
FWIW we don't have much testing of the C API in this repository itself, we currently rely on each of the language bindings repositories to do most of the testing.
#define WASMTIME_TRAP_CODE_UNREACHABLE_CODE_REACHED | ||
/// Execution has potentially run too long and may be interrupted. | ||
#define WASMTIME_TRAP_CODE_INTERRUPT | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it's ok to skip the enum { .. }
wrapper, and I think the value of each #define
constant will need to be listed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I had a bunch of #define
s first, then decided to model this after the config enums like
wasmtime/crates/c-api/include/wasmtime/config.h
Lines 56 to 64 in 73fd702
enum wasmtime_opt_level_enum { // OptLevel | |
/// Generated code will not be optimized at all. | |
WASMTIME_OPT_LEVEL_NONE, | |
/// Generated code will be optimized purely for speed. | |
WASMTIME_OPT_LEVEL_SPEED, | |
/// Generated code will be optimized, but some speed optimizations are | |
/// disabled if they cause the generated code to be significantly larger. | |
WASMTIME_OPT_LEVEL_SPEED_AND_SIZE, | |
}; |
...and didn't follow through properly 😬 I've updated the code using a proper enum instead of defines. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh I could go either way, I don't actually know enough about C to know whether these are really that different, and they feel like the same to me. I'm happy to defer to whichever you feel is more appropriate here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a C expert either -- it felt to be a tad more consistent going with the enum definition. Updated ✔️
crates/c-api/include/wasmtime/trap.h
Outdated
* Returns `true` if the trap is an instruction trap triggered while | ||
* executing Wasm. If `true` is returned then the trap code is returned | ||
* through the `code` pointer. If `false` is returned then this is not | ||
* an instruction trap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this describe a situation where false
is returned? (e.g. the trap was originally created with wasmtime_trap_new
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup will do
TrapCode::BadConversionToInteger => 8, | ||
TrapCode::UnreachableCodeReached => 9, | ||
TrapCode::Interrupt => 10, | ||
_ => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you leave a comment near enum TrapCode
and its #[non_exhaustive]
that any additions should get added here too?
ad76fb8
to
6cb2bd6
Compare
Eventually this should be added to the wasmtime-go binding, addressing bytecodealliance/wasmtime-go#63. Added a snippet to examples/interrupt.c to verify that this works as expected in manual testing. Signed-off-by: Stephan Renatus <[email protected]>
6cb2bd6
to
6e2a6a8
Compare
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This complements bytecodealliance/wasmtime#3086 Signed-off-by: Stephan Renatus <[email protected]>
This complements bytecodealliance/wasmtime#3086 Fixes bytecodealliance#63. Signed-off-by: Stephan Renatus <[email protected]>
This complements bytecodealliance/wasmtime#3086 Fixes #63. Signed-off-by: Stephan Renatus <[email protected]>
This complements bytecodealliance/wasmtime#3086 Fixes #63. Signed-off-by: Stephan Renatus <[email protected]>
Eventually this should be added to the wasmtime-go binding, addressing
bytecodealliance/wasmtime-go#63.
❓ How would this be tested, short of actually using it from a binding? Happy to add test cases, but I'm afraid I'll need some direction.