-
Notifications
You must be signed in to change notification settings - Fork 5k
Add few interpreter bringup diagnostic improvements #116699
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 few interpreter bringup diagnostic improvements #116699
Conversation
This change improves the diagnosability of the interpreter issues during the bringup. It prints out unsupported IL opcodes when we hit one and also makes the assert go through the assertAbort instead of the regular assert, since the regular one pops out a dialog, so it is bad for batch testing.
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.
Pull Request Overview
This pull request improves the interpreter's diagnostic output during bringup by printing unsupported IL opcodes and ensuring that assertions use a non-blocking abort mechanism during batch testing. Key changes include:
- Adding a custom assert macro in interpreter.h that calls assertAbort in debug builds.
- Introducing the function AssertOpCodeNotImplemented in compiler.cpp to report and abort on unsupported IL opcodes.
- Replacing raw assert(0) calls with calls to AssertOpCodeNotImplemented.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/coreclr/interpreter/interpreter.h | New custom assert macro defined for debug builds. |
src/coreclr/interpreter/compiler.cpp | Added AssertOpCodeNotImplemented and replaced raw asserts. |
fprintf(stderr, "IL_%04x %-10s - opcode not supported yet\n", | ||
(int32_t)(offset), | ||
CEEOpName(CEEDecodeOpcode(&ip))); | ||
assert(!"opcode not implemented"); |
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.
Consider providing a more descriptive error message in the assert call (e.g., including opcode value or context) to facilitate easier debugging when an unsupported opcode is encountered.
assert(!"opcode not implemented"); | |
assert((fprintf(stderr, "Assertion failed: IL_%04x %-10s - opcode not supported yet\n", (int32_t)(offset), CEEOpName(CEEDecodeOpcode(&ip))), false)); |
Copilot uses AI. Check for mistakes.
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
/ba-g the WASM build failed while compiling some file of the brotli stuff, but there were no details on what failed and that area was not touched by this pr. |
This change improves the diagnosability of the interpreter issues during the bringup. It prints out unsupported IL opcodes when we hit one and also makes the assert go through the
assertAbort
instead of the regular assert, since the regular one pops out a dialog, so it is bad for batch testing.Also, when we hit the case we set
m_hasInvalidCode
, assert.