ndarray: Fix four bugs in the modulo operator (% and %=)#749
Merged
Conversation
ndarray_binary_modulo allocated the result array as NDARRAY_UINT8 but then looped with uint16_t arithmetic via BINARY_LOOP. Because BINARY_LOOP writes through a uint16_t pointer but the array's itemsize is 1 byte, this caused out-of-bounds writes that corrupted both the result values and adjacent memory. The result dtype must be NDARRAY_UINT16 to match the left operand and to give BINARY_LOOP the correct element size.
Four bugs in the inplace modulo implementation: 1. ulab.h defined NDARRAY_HAS_INPLACE_MODU (typo, missing "LO"), so the NDARRAY_HAS_INPLACE_MODULO feature guard was never set and the entire ndarray_inplace_modulo function was compiled out. All %= operations silently fell back to binary % + rebind. 2. The function referenced larray and rarray in every INLINE_MODULO_FLOAT_LOOP call but never declared them, which would have been a compile error once the typo above was fixed. 3. The second dtype branch checked NDARRAY_UINT8 again instead of NDARRAY_INT8, making float %= int8 a no-op. 4. Only float lhs was handled; integer lhs returned the array unchanged instead of performing in-place integer modulo. Fix all four: correct the feature-guard typo, add the larray/rarray declarations, fix the NDARRAY_INT8 branch, and add full integer dispatch using INPLACE_LOOP with %=. The float path continues to use fmod via INLINE_MODULO_FLOAT_LOOP since C does not support %= on floating-point types. Integer lhs with float rhs raises TypeError, matching NumPy's in-place casting semantics.
- Add try/except TypeError around the inplace loop: integer lhs %= float rhs now raises TypeError (matching NumPy casting semantics) rather than silently rebinding via binary % fallback. - Update modulo.py.exp: binary uint16 % uint8 now shows correct values and dtype=uint16; inplace section now shows stable dtypes (true in-place for integer types) and the four expected TypeErrors. - Add a regression section with targeted cases for each of the four bugs: binary uint16 % uint8 dtype, inplace integer types, and float %= int8.
Contributor
Author
|
@v923z - We tried to integrate ulab back in October and ran into some bugs, then got busy. I came back to this and fixed all the issues. Can you check if it's good? I've done some trivial testing of it and using the new operator works now with this patch. Also, after this is merged, it would be great if you could release again. |
Owner
|
@kwagyeman Yes, it's good, many thanks for putting in the effort! |
Owner
|
@kwagyeman Here is the release tag: https://github.com/v923z/micropython-ulab/releases/tag/6.12. |
Contributor
Author
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The recently added modulo operator had four bugs, two of which caused
memory corruption or silent wrong results at runtime.
Binary
%—uint16 % uint8result dtypendarray_binary_moduloallocated the result array asNDARRAY_UINT8but then looped with
uint16_tarithmetic viaBINARY_LOOP. BecauseBINARY_LOOPwrites through auint16_tpointer but the array'sitemsizeis 1 byte, this caused out-of-bounds stores that corruptedboth the result values and adjacent memory. Fixed by allocating the
result as
NDARRAY_UINT16to match the left operand.Inplace
%=— four bugsFeature guard typo:
ulab.hdefinedNDARRAY_HAS_INPLACE_MODU(missing
LO), soNDARRAY_HAS_INPLACE_MODULOwas never set andndarray_inplace_modulowas compiled out entirely. All%=operations silently fell back to binary
%+ rebind.Missing variable declarations: Every
INLINE_MODULO_FLOAT_LOOPcall referenced
larrayandrarraybut they were never declared —a compile error waiting to happen once the typo above was fixed.
Duplicate dtype branch: The second
else ifcheckedNDARRAY_UINT8again instead ofNDARRAY_INT8, makingfloat %= int8a silent no-op.Integer lhs unhandled: Only
NDARRAY_FLOATlhs was dispatched;all integer lhs types fell through and returned the array unchanged.
Added full integer dispatch using
INPLACE_LOOPwith%=. Integerlhs
%=float rhs now raisesTypeError, matching NumPy's in-placecasting semantics.
Test plan
tests/2d/numpy/modulo.pyandmodulo.py.expto reflectcorrect dtypes and in-place semantics, including expected
TypeErrorfor integer
%=float