MatthieuDartiailh / bytecode

Python module to modify bytecode
https://bytecode.readthedocs.io/
MIT License
302 stars 38 forks source link

Fix stack effect #43

Closed thautwarm closed 6 years ago

thautwarm commented 6 years ago

Fix https://github.com/vstinner/bytecode/issues/38. @MatthieuDartiailh If dis.stack_effect doesn't convert oparg when unnecessarily, we wouldn't get this error.

https://github.com/python/cpython/blob/137b0632dccb992ca11e9445142fb33a29c33a51/Modules/_opcode.c#L38

However, for we cannot change the standard module immediately, we have to avoid passing the oparg when we didn't calculate the operand of an instruction.

For instance, Instr('LOAD_CONST', 0xFFFFFFFFFFFFFFF) might donate bytecode LOAD_CONST 1, and when we calculate its stack effect, we should use dis.stack_effect(100, 1) instead of dis.stack_effect(100, 0xFFFFFFFFFFFFFFF).

Actually we cannot convert 0xFFFFFFFFFFFFFFF to 1 when calculating stack effects, fortunately if we have to convert the oparg of Instr to the operand of bytecode instruction, it means that the oparg is insignificant to the result.

Thanks @serhiy-storchaka for https://github.com/python/cpython/blob/137b0632dccb992ca11e9445142fb33a29c33a51/Python/compile.c#L859.

codecov-io commented 6 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@22c8e98). Click here to learn what that means. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #43   +/-   ##
=========================================
  Coverage          ?   93.67%           
=========================================
  Files             ?       17           
  Lines             ?     2623           
  Branches          ?        0           
=========================================
  Hits              ?     2457           
  Misses            ?      166           
  Partials          ?        0
Impacted Files Coverage Δ
bytecode/instr.py 93.96% <100%> (ø)
bytecode/tests/test_instr.py 98.59% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 22c8e98...2dcd852. Read the comment docs.

SnoopyLane commented 6 years ago

For instance, Instr('LOAD_CONST', 0xFFFFFFFFFFFFFFF) might donate bytecode LOAD_CONST 1, and when we calculate its stack effect, we should use dis.stack_effect(100, 1) instead of dis.stack_effect(100, 0xFFFFFFFFFFFFFFF).

This is a subtle but critical detail. Thanks for the reminder!