mpy-cross: Make the debug emitter work again.

[agatti]

Summary

This PR fixes a regression introduced in 1b92bda when adding RV64 support, in which a new architecture was added to mpy-cross but it had no matching native emitter entry point table entry.

The result was that the architecture emitter entry point would be correctly calculated according to the native architecture index, but if the emitters entry points table was not updated to match the new number of architectures an out of bound access may be performed.

Unfortunately adding RV64IMC shifted the debug emitter index further down the table, that wasn't updated to reflect the lack of an emitter for RV64. Adding a NULL entry there would cause a NULL pointer access as there was no need to perform any check about the emitter entry point function's validity until now.

Testing

Running mpy-cross -X emit=native -march=debug <file> on current master would crash with a segment violation on Unix/x64. With the changes of this PR mpy-cross wouldn't crash any more and the compiled module output was printed to STDOUT. In addition, attempting to compile native code for RV64 triggers an error letting the user know there's no emitter for the chosen architecture.

To prevent situations like this in the future, a new test workflow has been added to make sure the debug emitter doesn't crash and reports output that seems correct. The test simply runs the debug emitter for tests/basics/0prelim.py and checks whether the command doesn't report an error and that the output contains an ENTRY and an EXIT line.

I thought about reusing .github/workflows/mpy_format.yml but then the trigger patterns would need to be extended to also cover py/** and mpy-cross/** making those checks run on more commits than needed.

Trade-offs and Alternatives

This could be implemented almost entirely within mpy-cross except for a new NULL entry in the emitter entry points table, although it is not a generic solution. Being generic, however, doesn't lessen the footprint impact on MicroPython's core. If that's unacceptable I can re-implement this PR to leave the compiler alone and add the necessary checks exclusively inside mpy-cross.

[github-actions]

Code size report:

Reference:  zephyr: Add support for GC split-heap. [3d9a3e8]
Comparison: github/workflows: Test mpy-cross debug emitter. [merge of 6b661ca]
  mpy-cross:   +80 +0.021% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    -4 -0.001% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +8 +0.001% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (3d9a3e8) to head (6b661ca).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #18366   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22299    22301    +2     
=======================================
+ Hits        21938    21940    +2     
  Misses        361      361           

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

[dpgeorge]

I thought about reusing .github/workflows/mpy_format.yml but then the trigger patterns would need to be extended to also cover py/** and mpy-cross/** making those checks run on more commits than needed.

I think that's OK to increase the trigger patterns for that existing workflow. Arguably, that should have been done anyway by e1b2f2e, because it made that CI job rely on building the unix port (and hence also mpy-cross).

So, I'd suggest reusing that workflow, instead of adding another one.

[agatti]

I see, thanks!

I've updated the commit in question so it extends .github/workflows/mpy_format.yml instead, and from what I can see said workflow didn't report any errors either.

[dpgeorge]

Alternatively, could add a dummy table for rv64 that raises a NotImplementedError in its new function. Then when/if it gets implemented the error case is removed.

(But that will add slightly more code size to mpy-cross...)

[agatti]

I'm fine either way, but I personally think having a NULL entry in the table isn't too much of a deal. This is restricted to mpy-cross, and there was a NULL entry in that table before this PR was submitted.

That said, regarding a RV64 emitter, I don't plan to implement such a thing myself in the near future, maybe somebody else can step up and leverage all the existing RV32 work. Granted, if I happen to have the need for such a thing or if there's enough folks asking for it - with nobody willing to get their hands dirty - then things may change. QEMU/RV64 also exists for situations like these.

[dpgeorge]

OK, let's leave it as you have it, with a NULL entry.

That said, regarding a RV64 emitter, I don't plan to implement such a thing myself in the near future

That's completely fine, there's no expectation at all in that regard 😄

I don't see much of a need for RV64 in the medium term future.