-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
py/objint,py/binary: Add int.to_bytes(signed) parameter, add common overflow checks. #16311
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
base: master
Are you sure you want to change the base?
py/objint,py/binary: Add int.to_bytes(signed) parameter, add common overflow checks. #16311
Conversation
77160cc
to
0fb050a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16311 +/- ##
==========================================
- Coverage 98.54% 98.36% -0.18%
==========================================
Files 169 169
Lines 21864 21892 +28
==========================================
- Hits 21545 21534 -11
- Misses 319 358 +39 ☔ View full report in Codecov by Sentry. |
Code size report:
|
c84ab58
to
88b516a
Compare
88b516a
to
140c2c9
Compare
4faf739
to
b3b3d4e
Compare
According to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Add `length`, `byteorder`, `signed` according to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Only two things left here...
|
Ah OK, there's a whole section about stack usage in the windows port README so I'm guessing we're just running out of stack. |
Add `length`, `byteorder`, `signed` according to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
For both small and long integers, raise an exception if calling struct.pack, adding an element to an array.array, or formatting an int with int.to_bytes would overflow the requested size. (Cherry-picked from CircuitPython commit 095c844.) Signed-off-by: Angus Gratton <angus@redyak.com.au>
Cherry-picked from CircuitPython commit d103ac1. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Cherry-picked from CircuitPython commit c592bd6. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Signed-off-by: Angus Gratton <angus@redyak.com.au>
Fixes case where destination nbytes is non-power-of-2 size. Also adds tests. Cherry-picked from CircuitPython commit 8664a65. Signed-off-by: Angus Gratton <angus@redyak.com.au>
- CPython and CircuitPython both raise OverflowError if an array constructor passes an out of bounds value. - MicroPython V1.x truncates the integer to suit. - The plan is for MicroPython V2 to change this to be the same as CircuitPython. The bounds checking cherry-picked from CircuitPython in 50bd33b and d352a73 adds these checks to array constructors. Move them behind a macro guard, and also rewrite the tests from CircuitPython to pass on MicroPython V1.x (but should be easy to convert over for MP2 in the future). This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Refactors similar code paths to a common mp_obj_int_to_bytes() function to reduce code size. This commit should have no functional changes. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au> Signed-off-by: Angus Gratton <angus@redyak.com.au>
Expands tests to match, although I think there may still be some corner cases where this doesn't work as expected on V2. Should be no change on V1. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au> Signed-off-by: Angus Gratton <angus@redyak.com.au>
Signed-off-by: Angus Gratton <angus@redyak.com.au>
This seems like it's only really a problem on Debug builds, but I think can't hurt to increase it on all windows builds. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Had a quick look into why stm32 got bigger while all the other bare metal ports got smaller. Didn't find anything unusual: no significant new symbols are being linked compared to master. Several functions shrunk as they now call a common helper. I think maybe they just didn't shrink as much as on the other bare metal ports... Diff of linked symbol sizes--- ./build-PYBV10-master/firmware.syms 2025-03-04 15:59:56.145991980 +1100
+++ ./build-PYBV10/firmware.syms 2025-03-04 16:00:08.656797386 +1100
@@ -5948,0 +5949 @@
+00000018 t allowed_args.0
@@ -6199 +6200 @@
-00000264 t compile_scope_inline_asm
+00000260 t compile_scope_inline_asm
@@ -6503 +6504 @@
-00000970 t emit_inline_thumb_op
+00000978 t emit_inline_thumb_op
@@ -7326 +7327 @@
-000000d8 t int_to_bytes
+0000005c t int_to_bytes
@@ -7770,3 +7771,3 @@
-0000002a T mp_binary_set_int
-00000114 T mp_binary_set_val
-0000008c T mp_binary_set_val_array
+0000005a T mp_binary_set_int
+000000f2 T mp_binary_set_val
+0000006e T mp_binary_set_val_array
@@ -8329 +8330 @@
-00000042 T mp_obj_exception_attr
+00000040 T mp_obj_exception_attr
@@ -8373 +8374 @@
-00000016 T mp_obj_int_to_bytes_impl
+00000114 T mp_obj_int_to_bytes
@@ -8521 +8522 @@
-0000091c T mp_qstr_const_hashes
+00000922 T mp_qstr_const_hashes
@@ -8523 +8524 @@
-0000048e T mp_qstr_const_lengths
+00000491 T mp_qstr_const_lengths
@@ -8525 +8526 @@
-00001250 T mp_qstr_const_pool
+0000125c T mp_qstr_const_pool
@@ -8891 +8892 @@
-000000d2 T mpz_as_bytes
+0000009e T mpz_as_bytes
@@ -9768 +9769 @@
-00000084 t set_aligned
+00000080 t set_aligned
@@ -10078 +10079 @@
-0000007c t task_attr
+00000078 t task_attr
@@ -10216 +10217 @@
-00000200 t uctypes_struct_attr_op
+00000204 t uctypes_struct_attr_op (Files generated with I don't see much we can do to improve this. Interestingly, the firmware size increase on PYBV10 is about the same if LTO is enabled (although the firmwares are overall smaller.) |
2c3f941
to
6f3c7e0
Compare
What about: -00000016 T mp_obj_int_to_bytes_impl
+00000114 T mp_obj_int_to_bytes That's 254 extra bytes. Is that similar on other ports that did decrease? |
I think so, this is the common function that the others now call. They grow by basically the same amount on both samd and rp2 ports. Here's samd: @@ -6342 +6338 @@
-00000016 T mp_obj_int_to_bytes_impl
+00000114 T mp_obj_int_to_bytes Full size diffs--- build-ADAFRUIT_ITSYBITSY_M4_EXPRESS-master/firmware.syms 2025-03-04 17:06:29.044966391 +1100
+++ build-ADAFRUIT_ITSYBITSY_M4_EXPRESS/firmware.syms 2025-03-04 17:07:24.983870265 +1100
@@ -993 +992,0 @@
-t $d
@@ -4381,3 +4379,0 @@
-t $t
-t $t
-t $t
@@ -4404,0 +4401 @@
+00000018 t allowed_args.0
@@ -4620 +4617 @@
-00000260 t compile_scope_inline_asm
+00000264 t compile_scope_inline_asm
@@ -4822 +4819 @@
-00000968 t emit_inline_thumb_op
+00000970 t emit_inline_thumb_op
@@ -5312 +5309 @@
-000000d8 t int_to_bytes
+0000005c t int_to_bytes
@@ -5758,4 +5755,3 @@
-0000002a T mp_binary_set_int
-0000016c T mp_binary_set_val
-0000008c T mp_binary_set_val_array
-000000f2 T mp_binary_set_val_array_from_int
+0000005a T mp_binary_set_int
+00000150 T mp_binary_set_val
+0000006e T mp_binary_set_val_array
@@ -5777 +5773 @@
-0000008c t mp_builtin_compile
+00000088 t mp_builtin_compile
@@ -6300 +6296 @@
-0000003a T mp_obj_exception_attr
+00000038 T mp_obj_exception_attr
@@ -6342 +6338 @@
-00000016 T mp_obj_int_to_bytes_impl
+00000114 T mp_obj_int_to_bytes
@@ -6477 +6473 @@
-00000542 T mp_qstr_const_hashes
+00000536 T mp_qstr_const_hashes
@@ -6479 +6475 @@
-000002a1 T mp_qstr_const_lengths
+0000029b T mp_qstr_const_lengths
@@ -6481 +6477 @@
-00000a9c T mp_qstr_const_pool
+00000a84 T mp_qstr_const_pool
@@ -6802 +6798 @@
-000000d2 T mpz_as_bytes
+0000009e T mpz_as_bytes
@@ -7172 +7168 @@
-00000084 t set_aligned
+00000080 t set_aligned
@@ -7245 +7241 @@
-00000034 t slice_attr
+00000030 t slice_attr
@@ -7402 +7398 @@
-00000070 t task_attr
+0000007c t task_attr
@@ -7553 +7549 @@
-00000200 t uctypes_struct_attr_op
+00000204 t uctypes_struct_attr_op
@@ -7599 +7595 @@
-00000038 t usb_device_attr
+00000034 t usb_device_attr --- build-RPI_PICO-master/firmware.syms 2025-03-04 17:01:17.898379768 +1100
+++ build-RPI_PICO/firmware.syms 2025-03-04 17:01:28.760234428 +1100
@@ -31,4 +30,0 @@
-t $d
-t $d
-t $d
-t $d
@@ -244 +239,0 @@
-t $d
@@ -630,0 +626,2 @@
+t $d
+r $d
@@ -1202,0 +1200 @@
+t $d
@@ -1536,0 +1535 @@
+t $d
@@ -5462,3 +5460,0 @@
-t $t
-t $t
-t $t
@@ -5503,0 +5500 @@
+00000018 r allowed_args.0
@@ -5826 +5823 @@
-00000278 t compile_scope_inline_asm
+00000274 t compile_scope_inline_asm
@@ -6144 +6141 @@
-00000498 t emit_inline_thumb_op
+00000494 t emit_inline_thumb_op
@@ -6781 +6778 @@
-000000d4 t int_to_bytes
+00000060 t int_to_bytes
@@ -6786 +6783 @@
-00000044 t iobase_ioctl
+00000040 t iobase_ioctl
@@ -7352,4 +7349,3 @@
-0000002a T mp_binary_set_int
-00000154 T mp_binary_set_val
-00000084 T mp_binary_set_val_array
-00000084 T mp_binary_set_val_array_from_int
+00000056 T mp_binary_set_int
+0000012e T mp_binary_set_val
+00000064 T mp_binary_set_val_array
@@ -7370 +7366 @@
-00000064 t mp_builtin_compile
+00000068 t mp_builtin_compile
@@ -7885 +7881 @@
-0000005c T mp_native_type_from_qstr
+00000064 T mp_native_type_from_qstr
@@ -7962 +7958 @@
-00000016 T mp_obj_int_to_bytes_impl
+00000110 T mp_obj_int_to_bytes
@@ -8109 +8105 @@
-000002ee R mp_qstr_const_hashes
+000002f1 R mp_qstr_const_hashes
@@ -8111 +8107 @@
-000002ee R mp_qstr_const_lengths
+000002f1 R mp_qstr_const_lengths
@@ -8113 +8109 @@
-00000bd0 R mp_qstr_const_pool
+00000bdc R mp_qstr_const_pool
@@ -8355 +8351 @@
-00000064 T mp_vfs_blockdev_init
+00000060 T mp_vfs_blockdev_init
@@ -8365 +8361 @@
-00000068 T mp_vfs_getcwd
+0000006c T mp_vfs_getcwd
@@ -8367,2 +8363,2 @@
-00000060 T mp_vfs_ilistdir
-00000070 t mp_vfs_ilistdir_it_iternext
+00000064 T mp_vfs_ilistdir
+00000074 t mp_vfs_ilistdir_it_iternext
@@ -8370 +8366 @@
-0000007c T mp_vfs_import_stat
+0000007a T mp_vfs_import_stat
@@ -8433 +8429 @@
-0000002e T mp_vfs_rename
+00000030 T mp_vfs_rename
@@ -8435 +8431 @@
-00000016 T mp_vfs_rmdir
+00000018 T mp_vfs_rmdir
@@ -8437 +8433 @@
-00000040 T mp_vfs_stat
+0000003c T mp_vfs_stat
@@ -8439 +8435 @@
-00000054 T mp_vfs_statvfs
+00000058 T mp_vfs_statvfs
@@ -8441 +8437 @@
-0000009c T mp_vfs_umount
+000000a0 T mp_vfs_umount
@@ -8450 +8446 @@
-00000140 T mpz_as_bytes
+000000ea T mpz_as_bytes
@@ -9025 +9021 @@
-00000084 t set_aligned
+0000007c t set_aligned
@@ -9102 +9098 @@
-00000038 t slice_attr
+00000034 t slice_attr Ah! But the very big difference on these ports is that, unlike stm32, the That's the function which is still called from stm32/adc.c . So if we can find another way to implement that, the code would go down on stm32 (and we also wouldn't need the changes in #16858). |
EDIT: Accidentally buried the useful info in the |
Specifically pyb.ADC.read_timed and read_timed_multi are documented as supporting either an array of 8/16-bit integers or a bytearray.
|
return; | ||
} | ||
#endif | ||
mp_binary_set_val_array_from_int(typecode, p, index, mp_obj_get_int(val_in)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double checking re: discussion in comments above, this is the place that previously called mp_binary_set_val_array_from_int()
on all ports.
I think this is a valid change (and covered by tests), non-integer typecodes are handled above so all of the other possibilities are integers of different sizes.
We can probably call mp_binary_set_val_array
from adc.c as well, will need to test to be certain.
default: { | ||
size_t size = mp_binary_get_size('@', typecode, NULL); | ||
p = (uint8_t *)p + index * size; | ||
mp_obj_int_to_bytes(val_in, size, p, MP_ENDIANNESS_BIG, is_signed(typecode), OVERFLOW_CHECKS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This endianness choice seems wrong, though, at least mp_binary_set_val_array_from_int
would have used native endianness...? Need to make sure this is being tested.
According to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Add `length`, `byteorder`, `signed` according to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Support signed param: result = int.from_bytes(bytearray(), order='big'|'little', signed=False|True) Add `length`, `byteorder`, `signed` according to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Support signed param: result = int.from_bytes(bytearray(), order='big'|'little', signed=False|True) Add `length`, `byteorder`, `signed` according to the micropython#16311. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Summary
This PR is attempting to bring some CircuitPython changes upstream to MicroPython. From these PRs:
The changes:
int.to_bytes(signed=bool)
parameter. This removes the CPython incompatibility where currentlyint.to_bytes
in MicroPython behaves as ifsigned=self < 0
when converting.After cherry-picking commits directly from CircuitPython and resolving conflicts, some additional commits were added:
byteorder
andlength
, as per py/objint: Make byteorder argument optional in byte-conversion methods. #14387 and CPython >=3.11.OverflowError
from array constructors if a value is out of range, truncate the value instead. This is a documented difference between CPython and MicroPython, and after discussing with @dpgeorge decided changing it here has the potential to break existing MP code. The overflow checks are kept ifMICROPY_PREVIEW_VERSION_2
is set, so that we can change behaviour to match CPython (and CircuitPython) in MicroPython 2. The original justification for not checking here was to save code size, but the firmware code size will actually become smaller after making this change (due to a common code path for overflow checks in array constructors and int.to_bytes). I guess Python code size may become a littler bigger if masking needs to be added, but it also eliminates a potential silent bug.mp_binary_set_val_array_from_int
so the project coverage metric stays passing.Testing
Trade-offs and Alternatives