fix(evm): prevent C++ exception from crossing JIT frames in check mode#504
Merged
Conversation
⚡ Performance Regression Check Results✅ Performance Check Passed (interpreter)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions ✅ Performance Check Passed (multipass)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions |
Contributor
There was a problem hiding this comment.
Pull request overview
In "check mode" (ZEN_ENABLE_JIT && !ZEN_ENABLE_CPU_EXCEPTION), the previous code in EVMInstance::chargeGas/addGas threw a C++ exception across JIT-compiled frames when out of gas, which is undefined behavior. This PR converts the gas-accounting helpers to return a bool, propagates the failure through callers (expandMemory*, all of evm_imported.cpp's host helpers), and adds a JIT-side soft-error check (emitRuntimeSoftErrorCheck) that branches to a gas/static-violation trap BB after each potentially-failing host call. A new evmGetErrorCode runtime helper is exposed so the JIT can inspect the instance error code.
Changes:
- Convert
chargeGas/addGas/expandMemoryto returnbooland replacethrow/triggerInstanceExceptionOnJITwithsetInstanceExceptionOnJIT+ early return in check mode; update all runtime helpers to bail out onchargeGasfailure. - Add
callRuntimeForMayFailoverloads inEVMMirBuilderand switch most JIT-emitted host calls (BALANCE, SLOAD, SSTORE, KECCAK256, CALL/STATICCALL/DELEGATECALL/CALLCODE, CREATE/CREATE2, COPY, LOG, RETURN/REVERT/SELFDESTRUCT, EXTCODE*) to use it. - Introduce
evmGetErrorCodehost helper andemitRuntimeSoftErrorCheck, which compares the post-call error code againstGasLimitExceededandEVMStaticModeViolationand branches to the corresponding exception-set BB.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/runtime/evm_instance.h | Change expandMemory/chargeGas/addGas return types to bool. |
| src/runtime/evm_instance.cpp | Implement bool-returning gas helpers; in check mode, set instance error and return false instead of throwing. |
| src/compiler/evm_frontend/evm_imported.h | Declare evmGetErrorCode and add GetErrorCode to runtime function table. |
| src/compiler/evm_frontend/evm_imported.cpp | Propagate chargeGas/addGas boolean returns through all helpers; add evmGetErrorCode. |
| src/compiler/evm_frontend/evm_mir_compiler.h | Declare callRuntimeForMayFail overloads and emitRuntimeSoftErrorCheck. |
| src/compiler/evm_frontend/evm_mir_compiler.cpp | Implement callRuntimeForMayFail and the soft error check; switch most opcode handlers to use it. |
Comments suppressed due to low confidence (2)
src/runtime/evm_instance.cpp:300
- The
return false;on line 299 (after#endif) is unreachable in every preprocessor branch: the CPU-exception branch traps viatriggerInstanceExceptionOnJIT->ud2, the soft-exception branch already returns on line 295, and the no-JIT branch throws. It will likely produce -Wunreachable-code-return warnings under some configs and is inconsistent withaddGasbelow (which has no trailingreturn false;after#endif). Either drop the trailingreturn false;here or align the two functions.
#if defined(ZEN_ENABLE_JIT) && defined(ZEN_ENABLE_CPU_EXCEPTION)
triggerInstanceExceptionOnJIT(this, common::ErrorCode::GasLimitExceeded);
#elif defined(ZEN_ENABLE_JIT) && !defined(ZEN_ENABLE_CPU_EXCEPTION)
setInstanceExceptionOnJIT(this, common::ErrorCode::GasLimitExceeded);
return false;
#else
throw common::getError(common::ErrorCode::GasLimitExceeded);
#endif
return false;
}
src/runtime/evm_instance.cpp:321
- The preprocessor branches in
chargeGas(lines 291-299) andaddGas(lines 311-321) are ordered differently:chargeGasputs the CPU-exception branch first and has a trailingreturn false;after#endif, whileaddGasputs the soft branch first and has no trailing return. The behavior is equivalent, but the inconsistency is confusing and makes diffing/maintenance harder. Consider unifying the structure of these two helpers.
bool EVMInstance::addGas(uint64_t GasAmount) {
evmc_message *Msg = getCurrentMessage();
ZEN_ASSERT(Msg && "Active message required for gas accounting");
uint64_t GasLeft = getGas();
if (GasLeft > UINT64_MAX - GasAmount) {
#if defined(ZEN_ENABLE_JIT) && !defined(ZEN_ENABLE_CPU_EXCEPTION)
setInstanceExceptionOnJIT(this, common::ErrorCode::GasLimitExceeded);
return false;
#elif defined(ZEN_ENABLE_JIT) && defined(ZEN_ENABLE_CPU_EXCEPTION)
triggerInstanceExceptionOnJIT(this, common::ErrorCode::GasLimitExceeded);
return false;
#else
throw common::getError(common::ErrorCode::GasLimitExceeded);
#endif
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When ZEN_ENABLE_CPU_EXCEPTION=OFF (check mode), JIT-compiled code lacks DWARF unwind info, so C++ exceptions thrown from host API functions (e.g. chargeGas's throw on out-of-gas) cannot propagate through JIT frames and cause std::terminate. Fix by replacing all throw paths in host API call chains with setInstanceExceptionOnJIT + early return, and adding post-call error code checks in the compiler IR: - Add chargeGasHostApi() wrapper that pre-checks gas and uses setInstanceExceptionOnJIT instead of throw in JIT+check mode - Guard expandMemory/expandMemoryChecked with pre-check and markOutOfGas lambda (no-throw in JIT+check mode) - Add JIT+check branch in addGas() overflow handling - Add overflow pre-check before addGas(CALL_GAS_STIPEND) in evmHandleCallInternal - Add evmGetErrorCode host function and insert error-code checks after every callRuntimeFor in the MIR compiler, branching to GasLimitExceeded or EVMStaticModeViolation exception BBs
- Make chargeGas() and addGas() return bool; all three modes (interpreter, JIT+CPU_EXCEPTION, JIT+check) now handled internally, eliminating the chargeGasHostApi wrapper - Simplify expandMemory() to use chargeGas() return value instead of separate #if branches; remove markOutOfGas lambda from expandMemoryChecked() - Remove addGas(CALL_GAS_STIPEND) overflow pre-check in favor of addGas() returning false on overflow - Split callRuntimeFor into callRuntimeFor (pure reads) and callRuntimeForMayFail (may charge gas/fail), extracting emitRuntimeSoftErrorCheck as a shared helper
Rename callRuntimeForMayFail to callRuntimeForWithErrorCheck, keep TSTORE on checked runtime path, and document oversize-memory OOG signaling to make check-mode behavior explicit and safer to maintain.
cmgCr
force-pushed
the
cmgCr/fix_jit_check
branch
from
40bd256 to
5849547
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note