Test REPR_B & fix test failures #17688

jepler · 2025-07-15T17:57:32Z

Summary

For m68k-micropython I'm interested in REPR_B. However, CI doesn't check any REPR_B builds, so of course there were test failures.

This patch series adds a 32-bit REPR_B build (i686 Linux) and fixes the test failures that arose.

Testing

I ran the tests locally with a REPR_B build. I'll check the CI for other failures.

Trade-offs and Alternatives

Just for the benefit of one test in the testsuite, I added micropython.small_int_max. Instead, a few tests could be changed so that they just use numbers sized to work on "any repr that gets tested".

codecov · 2025-07-15T18:04:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.37%. Comparing base (6681530) to head (2a57986).
⚠️ Report is 72 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17688      +/-   ##
==========================================
- Coverage   98.38%   98.37%   -0.01%     
==========================================
  Files         171      171              
  Lines       22299    22306       +7     
==========================================
+ Hits        21939    21944       +5     
- Misses        360      362       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-07-15T18:06:57Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +56 +0.007% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

jepler · 2025-07-15T20:11:47Z

Native & viper tests fail if --via-mpy even after accounting for small int size. I did some digging and I'm guessing it's because mpy-cross uses a fixed representation for integers in the code emitter, e.g.,

        } else if (si->vtype == VTYPE_INT || si->vtype == VTYPE_UINT) {
            ASM_MOV_REG_IMM(emit->as, reg_dest, (uintptr_t)MP_OBJ_NEW_SMALL_INT(si->data.u_imm));

I chose to simply skip running any of the "native" tests, even though locally they succeed when not going via mpy. Because this was the easiest of the choices available.

dpgeorge · 2025-07-17T00:18:13Z

Just for the benefit of one test in the testsuite, I added micropython.small_int_max. Instead, a few tests could be changed so that they just use numbers sized to work on "any repr that gets tested".

I'd prefer not to add this constant, if we can help it. Could the tests automatically detect the max int? Note there is already sys.maxsize which may be useful. Could also add a feature check in tests/feature_check/ to detect the representation.

If we do need to add a constant, it might be better as sys.implementation._small_int.

jepler · 2025-07-18T02:29:21Z

I reworked it to not require the added property.

jepler · 2025-07-18T02:29:57Z

is this a known-to-be-flaky test? My changes shouldn't have touched it. 1 tests failed: extmod/select_poll_eintr.py

tests/basics/int_big_to_small.py

dpgeorge · 2025-07-18T04:58:30Z

is this a known-to-be-flaky test? My changes shouldn't have touched it. 1 tests failed: extmod/select_poll_eintr.py

Yes, it's known flaky. PR #17655 works around the issue.

jepler · 2025-07-18T14:10:53Z

The one remaining failure appears to be 1 tests failed: extmod/select_poll_eintr.py

jepler · 2025-09-14T22:48:58Z

@dpgeorge would it be easier if I split this up? The uctypes change to fit the integer width of REPR_B is the most important part for me in m68k-micropython. The float conversion bit is nice to have. The rest is "just" needed so that the tests of the added REPR_B unix build can pass.

extmod/moductypes.c

dpgeorge · 2025-09-15T12:43:12Z

extmod/moductypes.c

+// Here we need to set sign bit right
+#define TYPE2SMALLINT(x, nbits) ((((int)x) << (32 - nbits)) >> 2)
+#define GET_TYPE(x, nbits) (((x) >> (30 - nbits)) & ((1 << nbits) - 1))
+#define VALUE_MASK(type_nbits) (~((int)0xc0000000 >> type_nbits))


Can any of these constants be computed using values from py/smallint.h (eg MP_SMALL_INT_POSITIVE_MASK) so that the definitions can be common for both object repr's?

In principle, yes. In practice, I spent some time trying to do so and failed. I will try some more tomorrow, because simplifying this would be good. Failing that, I will comment it better.

Actually I don't think it's possible. Because on 64-bit targets we still want this to use 32-bit small-int ranges. I think that's so the constants are the same across "all" targets.

Well, now with this change they won't be, repr B will have different constants. I guess we could in principle change all targets to use the same, smaller, sizing. But probably that's not a good idea.

So... how you have it looks like the best way to do it.

I did go ahead and do this in a way that widened the values to be whatever mp_int_t can accommodate. I guess this is the cause of the slight growth in the x86_64 build?

extmod/moductypes.c

dpgeorge · 2025-09-15T12:47:56Z

tests/basics/int_big_to_small.py

+micropython.heap_lock()
+try:
+    k = (1 << 29)
+    k = k + 20


The compiler does a lot of constant folding, in particular the 1<<29 is done at compile time, not runtime. To make it clear that it's the add that may fail, I suggest:

k = 1 << 29 micropython.heap_lock() try: k += 1 except MemoryError: print("SKIP") raise SystemExit finally: micropython.heap_unlock()

dpgeorge · 2025-09-15T12:51:12Z

tests/basics/int_big_to_small_int29.py

@@ -0,0 +1,32 @@
+try:


Please add a comment that this test is specifically for repr B, where small ints are maximum (1<<29)-1 (and that it also works fine for other reprs).

Please add comment.

tests/perf_bench/core_import_mpy_multi.py

tests/perf_bench/core_import_mpy_single.py

dpgeorge · 2025-09-15T12:54:00Z

tests/stress/fun_call_limit.py


 # If the port has at least 32-bits then this test should pass.
-print(test(29))
+print(test(28))


I don't understand why this change is needed.

This test fails in REPR_B with the argument 29:

FAILURE /home/jepler/src/micropython/tests/results/stress_fun_call_limit.py --- /home/jepler/src/micropython/tests/results/stress_fun_call_limit.py.exp 2025-09-16 09:50:39.647389928 -0500 +++ /home/jepler/src/micropython/tests/results/stress_fun_call_limit.py.out 2025-09-16 09:50:39.647389928 -0500 @@ -1,2 +1,2 @@ -32 +SyntaxError SyntaxError make: Leaving directory '/home/jepler/src/micropython/ports/unix'

The test is if (i >= MP_SMALL_INT_BITS - 1) so the upper bound depends on the number of bits stolen by the obj_repr, not the actual number of bits in the underlying C type. Maybe this is another spot where MP_INT_BITS (introduced by me in this PR but local to uctypes) would be good to use, since this is about the C type not Python small ints.

OK, thanks for the reasoning.

since this is about the C type not Python small ints.

Actually, it is about small ints because star_args (which holds 1 << i) is encoded as a small int in the bytecode. So the C code is correct.

I suggest therefore:

change the comment above this line to something like "This is the biggest number that will pass on a 32-bit port using object representation B, which has 1<<28 still fitting in a positive small int."

the for i in range(30, 70): below should be changed to for i in range(28, 70):, so that it tests the transition from non-syntaxerror to syntaxerror (it should've been range(29,70) prior to this PR!)

dpgeorge · 2025-09-15T12:55:25Z

tools/ci.sh

+    VARIANT=coverage
+    CFLAGS_EXTRA="-DMICROPY_OBJ_REPR=MICROPY_OBJ_REPR_B -Dmp_int_t=int32_t -Dmp_uint_t=uint32_t"
+    MICROPY_FORCE_32BIT=1
+    RUN_TESTS_MPY_CROSS_FLAGS="--mpy-cross-flags=\"-march=x86 -msmall-int-bits=29\""


Shouldn't it be -msmall-int-bits=30?

yes, fixed.

dpgeorge · 2025-09-15T12:56:39Z

tools/ci.sh

+}
+
+function ci_unix_coverage_repr_b_run_tests {
+    ci_unix_run_tests_helper "${CI_UNIX_OPTS_REPR_B[@]}"


Can we use ci_unix_run_tests_full_helper here? Or at least ci_unix_run_tests_full_no_native_helper?

As noted above

Native & viper tests fail if --via-mpy even after accounting for small int size.

full_no_native still runs viper tests via mpy.

Ah, I see.

Your remark is correct, that viper makes assumptions about small int encoding.

Maybe add a comment to that effect here, so we know in the future why it doesn't test mpy? (And maybe it can be fixed one day...)

dpgeorge · 2025-09-15T12:58:04Z

would it be easier if I split this up?

No, it's best together to see exactly what's needed to get repr B working under CI.

The field encodings for OBJ_REPR_B need to be slightly different than for other representations, as there are only 30 bits (rather than 31) available for small integers. This accounts for about half of all the test failures with an OBJ_REPR_B build, using a commandline like (wrapped): ``` make -j$(nproc) -C ports/unix/ VARIANT=coverage \ MICROPY_FORCE_32BIT=1 \ CFLAGS_EXTRA="-DMICROPY_OBJ_REPR=MICROPY_OBJ_REPR_B \ -Dmp_int_t=int32_t -Dmp_uint_t=uint32_t" test ``` The correct OFFSET_BITS and other values can be determined by the number of lost bits between MP_SMALL_INT_BITS and the total number of bits in the underlying C integral type. Signed-off-by: Jeff Epler <[email protected]>

jepler · 2025-09-17T13:16:21Z

[actually pushed now]

Different results are needed for different integer sizes. Signed-off-by: Jeff Epler <[email protected]>

Signed-off-by: Jeff Epler <[email protected]>

The check for 'fits in a small int' is specific to the 31-bit int of other OBJ_REPRs. Signed-off-by: Jeff Epler <[email protected]>

This showed up some interesting errors (hopefully all fixed now). Signed-off-by: Jeff Epler <[email protected]>

dpgeorge · 2025-09-18T04:43:17Z

extmod/moductypes.c

+// Bit 0 is "is_signed", other bits give log2 of scalar size
 #define GET_SCALAR_SIZE(val_type) (1 << ((val_type) >> 1))
-#define VALUE_MASK(type_nbits) ~((int)0x80000000 >> type_nbits)
+#define VALUE_MASK(type_nbits) ~(MP_SMALL_INT_MIN >> type_nbits)


It's nice to have these constants generalised like this. But, I think it's a (subtle) breaking change: this will change uctypes constant values and those can be embedded into .mpy files because the parser can optimise them to their int values (see mp_constants_table).

So, eg, someone using uctypes in a compiled .mpy file on a 64-bit arch, it won't work as expected after this change.

This change also means that, moving forward, building a 32-bit .mpy file on a 64-bit arch will use large ints for the substituted constants, rather than small ints. So it'll be suboptimal.

It's annoying, but we are stuck here.

Maybe the simplest way to handle this is just hard-code values like this:

#define MP_INT_BITS (32) #define MP_UINT_MAX (0xffffffff) #define MP_INT_MAX ((mp_int_t)(MP_UINT_MAX / 2)) #define MP_INT_MIN (-MP_INT_MAX - 1)

Upon further thought, this just isn't going to work at all. Consider the situation where mpy-cross builds a .mpy file using uctypes. That will use 31-bit small ints to encode these uctypes constants. Then if that .mpy is loaded on a obj-repr-B system the constant won't match the constant values used by that target.

There's not really an easy way to fix this. Either we need to change the constants for all targets so they fit on repr B. Or we need to have a way of storing in the .mpy which bit size was used. Both those options require a bump to the .mpy version (and the latter option is pretty tricky).

The main point here is, if we want to allow saving of .mpy files that use uctypes, the constants must be the same across all platforms.

Some ways to make it work for repr B, or work around the issue :

Store the constants as big ints, instead of small ints; see eg sys.maxsize for how to do that. I tried this and it can work, but it requires quite a few changes (eg MP_OBJ_SMALL_INT_VALUE -> mp_obj_get_int_truncated everywhere in moductypes.c)

disable uctypes for obj repr B

disable loading of .mpy files for obj repr B

jepler · 2025-09-20T15:01:44Z

Thanks for all that background, I was unaware of mp_constants_table.

I think that for my purposes, I can accept the cost of storing these as long ints and the code/memory overhead of doing so. I'll spin a fresh PR that will use macros to switch between "generic integer" APIs specifically in the case of REPR_B and the SMALL_INT APIs for all other REPRs.

The other alternative I could think of would be to disable uctypes in REPR_B builds here in micropython and I just carry the incompatible mpy constant changes over in m68k-micropython.

I'm good with either of those alternatives, but honestly as I think about it the 2nd is almost preferable (and I can put back REPR_B uctypes testing over in my m68k-micropython fork).

jepler · 2025-09-28T14:06:09Z

Closing in favor of #18109

jepler force-pushed the test-repr-b branch from cb17ed8 to 3b37e56 Compare July 15, 2025 20:08

jepler force-pushed the test-repr-b branch 2 times, most recently from 5a8c677 to 5dcf5fa Compare July 16, 2025 22:47

jepler force-pushed the test-repr-b branch 2 times, most recently from 9c67dca to 1bb4e0f Compare July 18, 2025 02:25

dpgeorge reviewed Jul 18, 2025

View reviewed changes

tests/basics/int_big_to_small.py Show resolved Hide resolved

jepler force-pushed the test-repr-b branch 2 times, most recently from fb4d2d4 to 66a33e4 Compare July 18, 2025 11:17

jepler requested a review from dpgeorge July 24, 2025 11:23

jepler force-pushed the test-repr-b branch from 66a33e4 to ec07a71 Compare July 25, 2025 18:27

dpgeorge added py-core Relates to py/ directory in source port-unix tests Relates to tests/ directory in source labels Aug 5, 2025

dpgeorge reviewed Sep 15, 2025

View reviewed changes

jepler force-pushed the test-repr-b branch from ec07a71 to b3724f1 Compare September 16, 2025 13:58

jepler force-pushed the test-repr-b branch 2 times, most recently from 370dbbf to 2f17887 Compare September 17, 2025 13:15

int_big_to_small: Bifurcate test using small_int_max.

0eebd30

Different results are needed for different integer sizes. Signed-off-by: Jeff Epler <[email protected]>

jepler added 4 commits September 17, 2025 09:30

tests: Use OBJ_REPR_B compatible mpy file headers.

36fef3c

Signed-off-by: Jeff Epler <[email protected]>

fun_call_limit: Allow to run with REPR_B.

383d03e

Signed-off-by: Jeff Epler <[email protected]>

objint: Fix converting float to int on REPR_B.

5366a02

The check for 'fits in a small int' is specific to the 31-bit int of other OBJ_REPRs. Signed-off-by: Jeff Epler <[email protected]>

ci: Add 32-bit REPR_B build.

2a57986

This showed up some interesting errors (hopefully all fixed now). Signed-off-by: Jeff Epler <[email protected]>

jepler force-pushed the test-repr-b branch from 14ca648 to 2a57986 Compare September 17, 2025 14:30

jepler mentioned this pull request Sep 17, 2025

mpprint: Correctly format leading zeros with separators. #18092

Closed

dpgeorge reviewed Sep 18, 2025

View reviewed changes

This was referenced Sep 20, 2025

uctypes incompatibility with OBJ_REPR_B #18105

Open

mpy-cross viper incompatibility with OBJ_REPR_B #18108

Open

Test REPR_B & fix test failures (w/o uctypes) #18109

Closed

jepler closed this Sep 28, 2025

Uh oh!

Test REPR_B & fix test failures #17688

Test REPR_B & fix test failures #17688

Uh oh!

Conversation

jepler commented Jul 15, 2025

Summary

Testing

Trade-offs and Alternatives

Uh oh!

codecov bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jepler commented Jul 15, 2025

Uh oh!

dpgeorge commented Jul 17, 2025

Uh oh!

jepler commented Jul 18, 2025

Uh oh!

jepler commented Jul 18, 2025

Uh oh!

Uh oh!

dpgeorge commented Jul 18, 2025

Uh oh!

jepler commented Jul 18, 2025

Uh oh!

jepler commented Sep 14, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jepler Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgeorge commented Sep 15, 2025

Uh oh!

jepler commented Sep 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 15, 2025 •

edited

Loading

github-actions bot commented Jul 15, 2025 •

edited

Loading

jepler Sep 16, 2025 •

edited

Loading