Skip to content
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

Fix stack effect #43

Merged

Conversation

thautwarm
Copy link
Contributor

Fix #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
Copy link

codecov-io commented Oct 16, 2018

Codecov Report

❗ 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
Copy link
Contributor

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!

Copy link
Owner

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comment edits suggestion (this new Github feature is great !) but otherwise this is good to go. Please check the length of the lines of my edits github does not provide yet rulers.

bytecode/tests/test_instr.py Outdated Show resolved Hide resolved
bytecode/tests/test_instr.py Outdated Show resolved Hide resolved
bytecode/tests/test_instr.py Outdated Show resolved Hide resolved
bytecode/tests/test_instr.py Outdated Show resolved Hide resolved
bytecode/instr.py Outdated Show resolved Hide resolved
bytecode/instr.py Outdated Show resolved Hide resolved
@MatthieuDartiailh MatthieuDartiailh merged commit a88e0d4 into MatthieuDartiailh:master Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants