py/emitnative: Fix case of clobbered REG_TEMP0 when loading const obj. #15573
Conversation
dpgeorge
added
py-core
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15573 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 161 161
Lines 21281 21281
=======================================
Hits 20948 20948
Misses 333 333 ☔ View full report in Codecov by Sentry. |
|
Code size report: |
dpgeorge
removed
the
board-definition
jimmo
left a comment
jimmo
left a comment
There was a problem hiding this comment.
TODO: would be good to improve the qemu-riscv (and qemu-arm) ports so they can test this case by generating .mpy files using mpy-cross with a native emitter for all their tests, and running the .mpy. But that requires a bit of upfront leg work.
Can you raise an issue describing this in more detail? (I'm not quite sure from the description above what needs to be done).
This change looks good otherwise, although given that this is not the first example of this sort of clobbering it would be good to know whether the suggested improvement to the qemu-testing is likely to be able to catch other cases?
The `emit_load_reg_with_object()` helper function will clobber `REG_TEMP0`. This is currently OK on architectures where `REG_RET` and `REG_TEMP0` are the same (all architectures except RV32), because all callers of `emit_load_reg_with_object()` use either `REG_RET` or `REG_TEMP0` as the destination register. But on RV32 these registers are different and so when `REG_RET` is the destination, `REG_TEMP0` is clobbered, leading to incorrectly generated machine code. This commit fixes the issue simply by using `REG_TEMP0` as the destination register for all uses of `emit_load_reg_with_object()`, and adds a comment to make sure the caller of this function is careful. Signed-off-by: Damien George <[email protected]>
dpgeorge
force-pushed
the
py-emitnative-fix-clobbered-temp0
branch
from
8155655 to
afba3e0
Compare
See #15609 which sets the stage to run native .mpy tests on the qemu-* ports.
We currently only run full native tests under CI for the x86 and x64 emitters. Enabling all the native tests under CI using qemu-arm and qemu-riscv, it's highly likely that would expose some of the bugs that we already found and fixed (which were found by running native tests on hardware, mostly ARM Thumb and Xtensa hardware). |
2 participants
Summary
The
emit_load_reg_with_object()helper function will clobberREG_TEMP0. This is currently OK on architectures whereREG_RETandREG_TEMP0are the same (all architectures except RV32), because all callers ofemit_load_reg_with_object()use eitherREG_RETorREG_TEMP0as the destination register. But on RV32 these registers are different and so whenREG_RETis the destination,REG_TEMP0is clobbered, leading to incorrectly generated machine code.This commit fixes the issue simply by using
REG_TEMP0as the destination register for all uses ofemit_load_reg_with_object(), and adds a comment to make sure the caller of this function is careful.Testing
This is currently tested only by inspecting output of
mpy-cross -march=debugfor the following code:See #15551 (comment) for background.
TODO: would be good to improve the qemu-riscv (and qemu-arm) ports so they can test this case by generating .mpy files using mpy-cross with a native emitter for all their tests, and running the .mpy. But that requires a bit of upfront leg work.