Skip to content

coverage: Get coverage of the various code emitters. #18002

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

coverage: Get coverage of the various code emitters. #18002

wants to merge 7 commits into from
+7,545 −43

Conversation

jepler
Copy link
Contributor

@jepler jepler commented Aug 29, 2025

Summary

I identified that the various code emitters were not well covered during CI (in terms of codecov or running with sanitizers), as well as some code paths that are specific to mpy-cross. I made a failed attempt to sanitize & cover the mpy-cross program in #17997, where it was suggested to make the code paths reachable from inside the coverage build.

I hope that this test will increase coverage, at least in lines. It might decrease coverage in percentage of coverable lines because there will be new coverable files and lines added.

Testing

I ran the added tests locally. CI will run them under more circumstances, as well as under UBsan and Asan.

Trade-offs and Alternatives

I initially tried to share code between mpy-cross/main.c and the new compiler-wrapper, but gave up on this.

I observed that native code always had a lot of zeros in it, because it is rounded up to page size when mmap()'d. I re-organized the way that the size of a mmap'd area is recorded so that the actual size can still be used.

I initially printed the size of mpy-compiled files and ran the debug emitter. In the end I removed this, as it's probably fragile and (in the case of the debug emitter) extremely verbose. Consequently, the debug emitter is not actually tested at all (and maybe should be disabled)

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 29, 2025 •
edited
Loading

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +24 +0.003% 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
Copy link
Contributor Author

jepler commented Aug 29, 2025

well that's interesting. Almost everything fails when using native code. I reproduce the crash locally but don't understand it yet. It's not due to the change around mmapp'ing, I reverted that. While it can happen with mpy-cross it also happens with build-coverage/micropython -X emit=native basics/0prelim.py.

@jepler
Copy link
Contributor Author

jepler commented Aug 29, 2025

oh. something gets turned off by MICROPY_DYNAMIC_COMPILER.

#if (MICROPY_EMIT_NATIVE && !MICROPY_DYNAMIC_COMPILER) || MICROPY_ENABLE_DYNRUNTIME
extern const mp_fun_table_t mp_fun_table;
#elif MICROPY_EMIT_NATIVE && MICROPY_DYNAMIC_COMPILER
// In dynamic-compiler mode eliminate dependency on entries in mp_fun_table.
// This only needs to be an independent pointer, content doesn't matter.
extern const int mp_fun_table;
#endif

jepler added 3 commits August 29, 2025 09:08
@jepler
core: Separate DYNAMIC_COMPILER from NO_RUNTIME.
253de43 
This is necessary so that the coverage port can generate
code for other CPUs but still run its own tests with
native code.

Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
persistentcode: Gate MPY_FEATURE_ARCH by CPU defines.
eca2f57 
Otherwise, when multiple emitters are enabled, the wrong
one may be chosen.

Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
extmod: Add modmpycross.
2a49c09 
.. with supporting changes across the core.

Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
Copy link
Contributor Author

jepler commented Aug 29, 2025

The preprocessor block for selecting the correct native arch needs checking. The old version assumed only a single emitter was ever enabled, and that it would match the executing CPU. I added further defined() guards but they may not be correct.

@codecov Codecov
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

❌ Patch coverage is 87.35632% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.98%. Comparing base (704c70c) to head (1b63da9).

Files with missing lines Patch % Lines
extmod/modmpycross.c 87.20% 11 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #18002      +/-   ##
==========================================
- Coverage   98.38%   92.98%   -5.41%     
==========================================
  Files         171      183      +12     
  Lines       22296    24942    +2646     
==========================================
+ Hits        21937    23193    +1256     
- Misses        359     1749    +1390

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

@jepler
Copy link
Contributor Author

jepler commented Aug 29, 2025

There are some sanitizer finds in the tests (ubsan), as I had "hoped":

 Compiling viper_binop_comp_imm.py with armv7em EMIT_OPT_BYTECODE
+../../py/asmthumb.c:249:96: runtime error: left shift of negative value -1
+../../py/asmarm.c:51:19: runtime error: left shift of 14 by 28 places cannot be represented in type 'int'
+../../py/asmarm.c:455:27: runtime error: left shift of 14 by 28 places cannot be represented in type 'int'

The PR achieves e.g., 80% of coverage in asmthumb.c in contrast to no coverage "before".

jepler added 4 commits August 29, 2025 10:12
@jepler
tests: New tests to cover code emitters.
d60b19d 
Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
asmarm: Fix ubsan diagnostics.
e78cbdd 
Ths fixes the following type of diagnostic:
"runtime error: left shift of 14 by 28 places cannot
be represented in type 'int'" and ensures the resulting
32-bit value is correct.

Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
asmthumb: Fix ubsan diagnostics.
c458e9f 
This fixes the diagnostic "runtime error: left shift of negative
value -1" and ensures the correct instruction is assembled.

Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
emitinlinerv32: Fix UBsan diagnostics.
e3ca7e7 
Signed-off-by: Jeff Epler <jepler@gmail.com>
@jepler
Copy link
Contributor Author

jepler commented Aug 29, 2025

"emitinline" should also now get coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@jepler