fails to load very big constant numbers
Closed this issue · 11 comments
Hey, I come back again and ask for your help!
When I was loading very big numbers, I got an OverflowError
:
bc = Bytecode([Instr('LOAD_CONST', 0xFFFFFFFF), Instr('RETURN_VALUE')])
bc.to_code()
>> OverflowError: Python int too large to convert to C long
Let's avoid performing dis.stack_effect
when the instruction is something like LOAD_CONST
?
Thanks for the report. Could you make a PR adding LOAD_CONST to _stack_effects so that we avoid the offending call ? Add a test and an entry in the changlog.
Very glad to do that. I'll make it done.
I can't reproduce the error. What python version do you use?
I did not have time to try to reproduce but the line 34 in _opcode.c
seemed like a possible reason for the error:
oparg_int = (int)PyLong_AsLong(oparg);
@serhiy-storchaka This 0xFFFFFFFF
...
And I'm using Python 3.6.5 in Windows 10 x86_64.
@MatthieuDartiailh Yes, you're right! (int)PyLong_AsLong(oparg)
causes this, but note that standard compiler could even compile 0xFFFFFFFFFF
..
I've found the way to solve this problem, a few minutes.
I am not sure that specifying a whitelist for opcodes whose stack effect depends on its argument is a good approach. The list is not complete. It lacks following opcodes: BUILD_LIST
, BUILD_LIST_UNPACK
, BUILD_MAP_UNPACK
, BUILD_SET
, BUILD_SET_UNPACK
, BUILD_TUPLE
, BUILD_TUPLE_UNPACK
, BUILD_TUPLE_UNPACK_WITH_CALL
, CALL_FUNCTION_VAR
, CALL_FUNCTION_VAR_KW
, MAKE_CLOSURE
. Also in new Python versions new opcodes can be added, and the meaning of old opcodes can be changed.
Are there other opcodes that need a workaround besides LOAD_CONST
?
#41 addresses this.
Thanks for the ping @serhiy-storchaka . Actually I probably was in too much of a rush when reviewing the #43.
Looking through Python opcode and our implementation, I believe you are correct that only LOAD_CONST
can manipulate arbitrary objects. And limiting the workaround to this one seems safer. I will review your PR sometimes next week. I want in particular to understand the need for the modified tests.
Thanks again for catching this.
I have extracted the fix for this issue into separate PR #44.