Skip to content

py/asmthumb: Fix T3 encoding of conditional branches. #17946

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

Merged
merged 1 commit into from
Aug 19, 2025

Conversation

agatti
Copy link
Contributor

@agatti agatti commented Aug 17, 2025

Summary

This PR fixes the encoding of conditional branch opcodes emitted for ARMv7-M targets, when the emitter decides to use the T3 encoding for said operation.

Fields J1 and J2 are now present in the generated opcode word, along with correcting some minor issues in bitmasks and shifts computation.

This fixes #17940.

Testing

The full MicroPython test suite was run on a RP2350 (as in, regular, --via-mpy, and --via-mpy --emit-native).

The micropython/viper_large_jump.py test was run on ESP8266, ESP32, and ESP32C3 in the same three ways as the RP2350 just to make sure the newly added test won't fail.


Incidentally, when testing this I found out the micropython/viper_ptrX_store_boundary.py tests aren't aware of tests/run-tests.py ability to detect large tests. I guess I'd have to rewrite those three tests using unittest and bake in the expected results in the test files themselves.

Copy link

codecov bot commented Aug 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (22deeeb) to head (bd413d3).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #17946   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22297    22297           
=======================================
  Hits        21938    21938           
  Misses        359      359           

☔ 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.

Copy link

Code size report:

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

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Aug 19, 2025
This commit fixes the encoding of conditional branch opcodes emitted for
ARMv7-M targets, when the emitter decides to use the T3 encoding for
said operation.

Fields J1 and J2 are now present in the generated opcode word, along
with correcting some minor issues in bitmasks and shifts computation.

This fixes micropython#17940.

Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

I see that this is my fault, and in fact the offending code was in the very first commit of MicroPython 429d719 ! I really did mess up the calculation to encode the byte-offset, although at least the lower bits were correct, and I guess that's why it's gone unnoticed until now.

Also, the comment (now removed) was right, the macros needed testing 😂

@dpgeorge dpgeorge force-pushed the viper-fix-armv7m-large-jumps branch from 333612b to bd413d3 Compare August 19, 2025 02:49
@dpgeorge dpgeorge merged commit bd413d3 into micropython:master Aug 19, 2025
67 of 70 checks passed
@dpgeorge
Copy link
Member

Incidentally, when testing this I found out the micropython/viper_ptrX_store_boundary.py tests aren't aware of tests/run-tests.py ability to detect large tests. I guess I'd have to rewrite those three tests using unittest and bake in the expected results in the test files themselves.

This SKIP-TOO-LARGE option is a very new feature. Feel free to adjust those tests to properly skip when out-of-memory (although I don't think they need to be unittest tests, that would make them use even more memory).

@agatti
Copy link
Contributor Author

agatti commented Aug 19, 2025

This SKIP-TOO-LARGE option is a very new feature. Feel free to adjust those tests to properly skip when out-of-memory (although I don't think they need to be unittest tests, that would make them use even more memory).

Right now those tests generate compiled blocks on the fly, which makes them fail in the middle of execution on low-memory platforms (ie. ESP8266 without a prologue file that maps some flash into IRAM). unittest has the ability to either split a single test file into separate cases that may or may not be skipped on their own, or even provide subtests (in this case the address width in bits) for a single fixture.

I can look into making the test generate all compiled blocks upfront so it would fail right away and not require unittest for a clean run on the 8266, but it may make those tests more complex than they are now. Still, thanks for the remark, I'll see what I can do with this!

@agatti agatti deleted the viper-fix-armv7m-large-jumps branch August 19, 2025 10:30
@dpgeorge
Copy link
Member

My preference is always to keep things simple if possible (but at the same time be pragmatic).

And it would actually be good to improve these viper ptr tests, because I'm seeing them fail on small boards like ADAFRUIT_ITSYBITSY_M0_EXPRESS, eg see this Octoprobe output: https://reports.octoprobe.org/github_selfhosted_testrun_255/RUN-TESTS_STANDARD,a@5f2a-ADA_ITSYBITSY_M0/micropython_viper_ptr8_store_boundary.py.out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large Viper code causing hang/crash on RP2350 but OK on RP2040
2 participants