py: Fix handling of large number of *args, and add more tests #8472
Conversation
Signed-off-by: Damien George <damien@micropython.org>
|
@dlech this is a follow up to your work. I would appreciate a review, thanks! |
dpgeorge
force-pushed
the
py-fun-call-star-args-test
branch
from
3cfdeb9
to
0d37cab
Compare
tests/basics/fun_callstardblstar.py
Outdated
| @@ -34,3 +34,6 @@ def f2(*args, **kwargs): | |||
|
|
|||
|
|
|||
| f2(*iter(range(4)), **{'a': 1}) | |||
|
|
|||
| # case where *args is not a tuple/list and takes up most of the memory allocated for **kwargs | |||
| f2(*range(100), **{str(i): i for i in range(100)}) | |||
There was a problem hiding this comment.
range is a sequence, so by itself, it is not any different than tuple/list in this usage. This is why the test above has iter(range(...)) - it hides the __len__.
There was a problem hiding this comment.
This particular test crashed MicroPython prior to your changes. The point is that range(100) doesn't match the code path that tests if the type is &mp_type_list/&mp_type_tuple.
But maybe it is even better to wrap it in iter(...).
There was a problem hiding this comment.
You were right, it wasn't testing the correct code path on the new version. Now updated to use iter.
tests/stress/fun_call_limit.py
Outdated
|
|
||
|
|
||
| def test(n): | ||
| s = "f(" + ",".join(str(i) for i in range(n)) + ", *('a', 'b'), 'c', 'd')" |
There was a problem hiding this comment.
This might be a bit more legible using a format string (all of the quotes close to each other make me a bit cross-eyed).
| s = "f(" + ",".join(str(i) for i in range(n)) + ", *('a', 'b'), 'c', 'd')" | |
| s = "f({}, *('a', 'b'), 'c', 'd')".format(",".join(str(i) for i in range(n))) |
There was a problem hiding this comment.
updated, and changed the 1-char strings to numbers to make it easier to read
There were two issues with the existing code: 1. "1 << i" is computed as a 32-bit number so would overflow when executed on 64-bit machines (when mp_uint_t is 64-bit). This meant that *args beyond 32 positions would not be handled correctly. 2. star_args must fit as a positive small int so that it is encoded correctly in the emitted code. MP_SMALL_INT_BITS is too big because it overflows a small int by 1 bit. MP_SMALL_INT_BITS - 1 does not work because it produces a signed small int which is then sign extended when extracted (even by mp_obj_get_int_truncated), and this sign extension means that any position arg after *args is also treated as a star-arg. So the maximum bit position is MP_SMALL_INT_BITS - 2. This means that MP_OBJ_SMALL_INT_VALUE() can be used instead of mp_obj_get_int_truncated() to get the value of star_args. These issues are fixed by this commit, and a test added. Signed-off-by: Damien George <damien@micropython.org>
The values are always real objects, only the key can be MP_OBJ_NULL to indicate a **kwargs entry. Signed-off-by: Damien George <damien@micropython.org>
Signed-off-by: Damien George <damien@micropython.org>
dpgeorge
force-pushed
the
py-fun-call-star-args-test
branch
from
0d37cab
to
1dbf393
Compare
There were two issues with the existing code:
"1 << i" is computed as a 32-bit number so would overflow when executed on 64-bit machines (when mp_uint_t is 64-bit). This meant that *args beyond 32 positions would not be handled correctly.
star_args must fit as a positive small int so that it is encoded correctly in the emitted code. MP_SMALL_INT_BITS is too big because it overflows a small int by 1 bit. MP_SMALL_INT_BITS - 1 does not work because it produces a signed small int which is then sign extended when extracted (even by mp_obj_get_int_truncated), and this sign extension means that any position arg after *args is also treated as a star-arg. So the maximum bit position is MP_SMALL_INT_BITS - 2. This means that MP_OBJ_SMALL_INT_VALUE() can be used instead of mp_obj_get_int_truncated() to get the value of star_args.
These are fixed by this PR.
Also, removed an unnecessary check for kw_value == MP_OBJ_NULL.
And added a few tests.