fix(compiler): refresh jit memory size for gas#325
Conversation
There was a problem hiding this comment.
Pull request overview
Updates EVM MIR compilation to reduce gas metering drift by normalizing offset/size handling for memory-touching operations and ensuring memory size used for expansion gas is refreshed from the instance.
Changes:
- Normalize offset+size pairs via
normalizeOffsetWithSize(...)for multiple memory-touching op handlers (avoids incorrect trapping / gas drift whensize == 0). - Use
UINT64_MAXfallback normalization for several runtime calls where non-64-bit operands should not immediately trap (e.g.,CALLDATALOAD,REVERT). - Change
getMemorySize()to load from the instance (and optionally update the cached variable) to avoid stale memory size during gas computation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifdef ZEN_ENABLE_EVM_GAS_REGISTER | ||
| reloadGasFromMemory(); | ||
| #endif | ||
| reloadMemorySizeFromInstance(); |
There was a problem hiding this comment.
handleMStore already updates the tracked memory size via expandMemoryIR(...) when expansion happens, and the store itself cannot change EVMInstance::MemorySize. The new reloadMemorySizeFromInstance() therefore adds an extra instance load with no observable effect. Consider removing it to avoid unnecessary work on a hot path, or document why mstore uniquely needs a post-op refresh.
| reloadMemorySizeFromInstance(); |
| MInstruction *MemSize = getInstanceElement(I64Type, MemorySizeOffset); | ||
| if (MemorySizeVar) { | ||
| createInstruction<DassignInstruction>(true, &(Ctx.VoidType), MemSize, | ||
| MemorySizeVar->getVarIdx()); | ||
| } |
There was a problem hiding this comment.
getMemorySize() now always loads the size from the instance and additionally writes it into MemorySizeVar, but MemorySizeVar is not read anywhere in this translation unit anymore. This both adds an extra dassign and may regress performance vs the previous cached-variable load. Consider either (a) restoring the cached-variable fast path by keeping MemorySizeVar as the canonical source and refreshing it only when runtime calls can grow memory, or (b) removing the MemorySizeVar updates if the intent is to always read from the instance.
| normalizeOperandU64(DestOffsetComponents); | ||
| // Use max uint64_t value if the offset/size is not 64-bit, because the | ||
| // returndatacopy will trigger memory access error instead of out-of-gas | ||
| // when offset/size is is very large. |
There was a problem hiding this comment.
Typo in comment: duplicated "is" in "when offset/size is is very large".
| // when offset/size is is very large. | |
| // when offset/size is very large. |
refresh memory size from the instance before expansion gas is computed normalize offset/size handling for memory-touching ops to avoid gas drift
refresh cached memory size after runtime log/codecopy to avoid drift remove redundant mstore reload and fix a comment typo
d2d15e9 to
fb39a35
Compare
refresh memory size from the instance before expansion gas is computed normalize offset/size handling for memory-touching ops to avoid gas drift
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