ports: Add RISC-V 32 bit QEMU port.

[agatti]

This adds a QEMU-based bare metal RISC-V 32 bits port. For the time being only QEMU's "virt" 32 bits board is supported, using the ilp32 ABI and the RV32IM architecture.

Tests run and pass except for two instances I'll debug and fix shortly, and the test generator framework was modified to remove hardcoded references to the "qemu-arm" port. If you want to test this on your machine I suggest you build your own toolchain from RISC-V's reference toolchain following these instructions. In my experience pre-built toolchains' support for 32 bits target is a hit and miss situation, ranging from missing headers to mismatched ABIs and more.

I'm not sure on whether this PR title (and commit message) is ok being prefixed with just "ports", as it touches several places and the list would be too long. If that's not ok let me know what to change that into to follow the repo's conventions.

This is part of the RISC-V work I'm doing on MicroPython; so instead of coming up with a very large and complicated patch to merge I'm splitting things up in more manageable chunks (the ESP32 Xtensa/RISC-V build changes in #12839 is part of this).

If this gets merged then the changes related to NLR/Native GC and the native code emitter (including compressed instructions support - RISC-V's equivalent of Arm's Thumb-2) can be tested on something more manageable than an ESP32-C3, and can also help for future RISC-V microcontroller ports. Plus if needed I can help to provide a PR to bring this into the CI as a build target.

The changes in tinytest.c are due to the fact that referencing stdout without libc being linked sort of confuses the compiler. The test framework isn't impacted otherwise.

By the way, is there any way to let the CI spellchecker know that certain words are not misspelled? Right now it complains about a register name in an __asm volatile block, for example.

[github-actions]

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% 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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.42%. Comparing base (d7d77d9) to head (1b10cb8).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #12853   +/-   ##
=======================================
  Coverage   98.42%   98.42%           
=======================================
  Files         161      161           
  Lines       21204    21204           
=======================================
  Hits        20870    20870           
  Misses        334      334           

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

[jimmo]

Thanks @agatti -- very helpful to add this as a first step!

By the way, is there any way to let the CI spellchecker know that certain words are not misspelled? Right now it complains about a register name in an __asm volatile block, for example.

See pyproject.toml

I'm not sure on whether this PR title (and commit message) is ok being prefixed with just "ports"

I think the way to solve this would be to move the improvements to tinytest/run-tests into their own commits.

e.g.

ports/qemu-riscv: Add RISC-V 32 bit QEMU port. 
tools/tinytest-codegen: Add support for port-specific generation.
tests/run-tests.py: Add support for riscv.

[agatti]

Thanks @jimmo, I'll split this PR up using your suggestion as a template. In the meantime I'll mark this PR as draft as it isn't ready for merging until all bits are merged first.

[jimmo]

Thanks @jimmo, I'll split this PR up using your suggestion as a template. In the meantime I'll mark this PR as draft as it isn't ready for merging until all bits are merged first.

Thanks @agatti -- splitting it into separate PRs wasn't actually necessary, just into separate commits in this PR would have been fine. But all good, thanks for doing it!

[agatti]

No worries, this also gives me the chance to decouple the various bits from the RISC-V port whenever possible and polish them up before merges.

[agatti]

In the meantime I cleaned up a few other things, like adding compressed instructions support, a proper CPU exception handler, and fixing the setvbuf crash when running tests without changing tinytest.

After that I looked into re-enabling failing tests from Makefile.test, but basics/bytearray_decode.py fails due to performing a string comparison between NULL and something else. I find this strange as other ports handle this case just fine, especially the bare metal Arm port, whose configuration I copied...

Incidentally this is the gdb backtrace for this case, is there anything special in the configuration I should look into (UTF8 validity checks aren't being done, but neither are in the Arm port...)?

#0  strncmp (s1=0x0, s2=0x802ecd04 "exp", n=0) at ../../shared/libc/string0.c:157
#1  0x80002b50 in qstr_find_strn (str=0x0, str_len=0) at ../../py/qstr.c:248
#2  0x8008c622 in mp_obj_new_str (data=0x0, len=0) at ../../py/objstr.c:2301
#3  0x80071d2a in mp_obj_str_make_new (type=0x80435f34 <mp_type_str>, n_args=2, n_kw=0, args=0x801d1a98) at ../../py/objstr.c:210
#4  0x8008ac04 in bytes_decode (n_args=2, args=0x801d1a98) at ../../py/objstr.c:1954
#5  0x8005e21c in fun_builtin_var_call (self_in=0x8043604c <bytes_decode_obj>, n_args=1, n_kw=0, args=0x801d1cdc) at ../../py/objfun.c:119
#6  0x80035756 in mp_call_function_n_kw (fun_in=0x8043604c <bytes_decode_obj>, n_args=1, n_kw=0, args=0x801d1cdc) at ../../py/runtime.c:712
#7  0x800361c4 in mp_call_method_n_kw (n_args=0, n_kw=0, args=0x801d1cd8) at ../../py/runtime.c:728
#8  0x800a7a56 in mp_execute_bytecode (code_state=0x801d1cc0, inject_exc=<optimized out>) at ../../py/vm.c:1042
#9  0x8005e682 in fun_bc_call (self_in=0x802d2a70 <heap+1768>, n_args=0, n_kw=0, args=0x0) at ../../py/objfun.c:273
#10 0x80035756 in mp_call_function_n_kw (fun_in=0x802d2a70 <heap+1768>, n_args=0, n_kw=0, args=0x0) at ../../py/runtime.c:712
#11 0x800356c4 in mp_call_function_0 (fun=0x802d2a70 <heap+1768>) at ../../py/runtime.c:686
#12 0x800bff12 in upytest_execute_test (
    src=0x80327be0 <pystr> "try:\n    print(bytearray(b'').decode())\n    print(bytearray(b'abc').decode())\nexcept AttributeError:\n    print(\"SKIP\")\n    raise SystemExit\n")
    at ../../shared/upytesthelper/upytesthelper.c:116
#13 0x800c661e in test_basics_bytearray_decode_py_fn (data=0x0) at build/genhdr/tests.h:4499
#14 0x800d123a in testcase_run_bare_ (testcase=0x80437d7c <_tests+1920>) at ../../lib/tinytest/tinytest.c:105
#15 0x800d1368 in testcase_run_one (group=0x8043b058 <groups>, testcase=0x80437d7c <_tests+1920>) at ../../lib/tinytest/tinytest.c:252
#16 0x800d1bd2 in tinytest_main (c=0, v=0x0, groups=0x8043b058 <groups>) at ../../lib/tinytest/tinytest.c:434
#17 0x800d1170 in main () at test_main.c:26

[dpgeorge]

It looks like the above null-dereference bug was introduced in 64c79a5, which added the call to strncmp().

According to the C standard, one should never pass NULL to strncmp() even if n (the length passed) is 0.

We could either add logic to handle the case of n==0, or adjust our implementation of strncmp() to allow this case of passing in NULL when the length is 0 (probably should do that regardless).

[jimmo]

Thanks @agatti @dpgeorge -- see fix here: #12894

[agatti]

Thank you @jimmo and @dpgeorge, I'll wait for #12859 and #12894 to land so I can update/rebase the PR and mark it as ready for merging on my end.

[dpgeorge]

Those PRs have now been merged.

[agatti]

Thanks @dpgeorge, I rebased the PR accordingly. Right now the extmod/deflate_decompress.py test is excluded as I still have to figure out if it's a compiler bug as it sometimes works. Rather than blocking this port any longer I put it in the exclusion list, to be removed from there later on.

Apparently, building the same code more than once with no changes will generate different binaries as (I guess) the test directories are not iterated in order, and different runs will generate different tests.h. Since entries are aligned, different orders generate different binary sizes, and some of them make the abovementioned test file break with:

Traceback (most recent call last):
  File "<stdin>", line 97, in <module>
  File "<stdin>", line 45, in decompress_error
  File "<stdin>", line 40, in decompress
MemoryError: memory allocation failed, allocating 32768 bytes
  
  FAIL ../../shared/upytesthelper/upytesthelper.c:128: Uncaught exception
result:
  [extmod/deflate_decompress.py FAILED]

at this point of the test:

micropython/tests/extmod/deflate_decompress.py

Line 97 in 958c6d9

decompress_error(data_gzip[:-8] + b"\x00\x00\x00\x00\x00\x00\x00\x00")

Now, since rebuilding toolchains takes a fair bit of time (and my laptop isn't the fastest out there either) it'll take me some time to bisect this, but at least all other tests work fine.

[dpgeorge]

building the same code more than once with no changes will generate different binaries as (I guess) the test directories are not iterated in order

The tests/run-tests.py test runner does sort the tests so they always run in the same order. Would be good to do the same when running the qemu-* tests.

Since entries are aligned, different orders generate different binary sizes, and some of them make the abovementioned test file break with:

It's strange it fails with a MemoryError... I thought each tests starts with a fresh heap and hence the same amount of free memory regardless of the order.

Does the test always pass if you double the amount of available heap? What about halving the heap?

[agatti]

Well, I was lucky and the first toolchain rebuild yielded a working compiler. There must be a bug in the optimiser for GCC 13.2, as GCC 12.2.0 works reliably (although builds still have different size depending on the tests order). I've updated the port's README file to reflect that.

If you have the chance to try this out yourselves let me know if you have any issues in setting the compiler up. A quick start guide on Linux (and I guess WSL too) is:

git clone https://github.com/riscv-collab/riscv-gnu-toolchain
cd riscv-gnu-toolchain
git checkout 2023.10.06
# "rv32imc_zicsr-ilp32--" only builds a compiler for just one arch/abi combination, the one used in the port makefile.  Not really needed but it takes less time overall.  Pick any path you want for the prefix, as long as the user that builds the toolchain can write into the new location.
./configure --with-multilib-generator="rv32imc_zicsr-ilp32--" --prefix=$HOME/riscv-gcc12 --enable-multilib
make -j `nproc`
make install
export PATH=$HOME/riscv-gcc12/bin:$PATH
cd $MICROPYTHON/ports/qemu-riscv
make -f Makefile.test test

[agatti]

building the same code more than once with no changes will generate different binaries as (I guess) the test directories are not iterated in order

The tests/run-tests.py test runner does sort the tests so they always run in the same order. Would be good to do the same when running the qemu-* tests.

I can come up with a PR for that.

Since entries are aligned, different orders generate different binary sizes, and some of them make the abovementioned test file break with:

It's strange it fails with a MemoryError... I thought each tests starts with a fresh heap and hence the same amount of free memory regardless of the order.

Does the test always pass if you double the amount of available heap? What about halving the heap?

Heap size changes did not change anything, neither stack did - however debug builds always worked. Anyway, that's now sorted, thanks for the pointer - I got the notification whilst I was typing the toolchain build instructions :)

[agatti]

PR for sorted test generation sent: #12906.

[dpgeorge]

Please add a license header comment.

[agatti]

Ok, that is the exact same file as qemu-arm's and it doesn't have a licence header either. That file was added in 5130b81 but then it was worked on by quite a few people. Whose copyright attribution shall I use for that, the author of the commit I mentioned before?

[dpgeorge]

If it's the exact same as qemu-arm then I would suggest to remove this file and just refer to the one in qemu-arm in the Makefile here. That way there is no duplication of code, and we can fix the licensing header separately.

Same thing for test_main.c, if that's exactly the same.

[projectgus]

Long term maybe we can add shared/qemu-common or a single ports/qemu?

(However agree for now that if most of the duplicate code is 100% the same, can simply refer to its current location from here.)

[dpgeorge]

I managed to get this running (at least the basic hello world) using the default toolchain on Arch Linux, riscv64-elf-gcc. Although I did need to explicitly specify the correct libgcc.a.

This new port will (eventually) need CI. Do you feel like adding that now?

[agatti]

This new port will (eventually) need CI. Do you feel like adding that now?

If you don't mind I'll look into this once the other review points have been addressed. Whether that is going to be a further commit to this PR or a whole new PR for me is the same, as long as it's convenient for you.

[projectgus]

Thanks for submitting this, @agatti! It'll be invaluable to have a way to run tests against a RISC-V target on the host.

I left some comments inline, mostly about the toolchain setup and the fiddly details of gcc multilib.

The only other significant thing a new port will need is to build in CI, and preferably also test in CI (see tools/ci.sh and .github/workflows/ports_qemu-arm.yml for reference) . Hopefully that will become pretty straightforward if prebuilt toolchains can be supported without any niggles. If this becomes a sticking point then let me know and one of us can probably help out.

[projectgus]

Long term maybe we can add shared/qemu-common or a single ports/qemu?

(However agree for now that if most of the duplicate code is 100% the same, can simply refer to its current location from here.)

[agatti]

Thank you @projectgus for taking the time to look at this. I'll answer your concerns inline, then once those are sorted out, I'll force push something that can build under CI.

[agatti]

Broken force push aside, I'm wondering why there's a test failure:

basics/string_fstring_invalid.py: 
Traceback (most recent call last):
  File "<stdin>", line 4
SyntaxError: invalid syntax

  FAIL ../../shared/upytesthelper/upytesthelper.c:128: Uncaught exception
result: 
  [basics/string_fstring_invalid.py FAILED]

That test did pass back when it was still part of a bigger test file, and the code on my end is the same - including the compiler. Is there anything special I should look into to see what's going on?

[agatti]

Ok, I've figured out how to dump the rules as they are parsed for the single statement print(f"\\"):

Arm dump
RISC-V dump

They diverge at line 137:

--- arm.txt         2024-05-31 16:58:09.741802105 +0200
+++ riscv.txt       2024-05-31 16:58:31.433929684 +0200
@@ -134,6 +134,6 @@
 depth=37                                      trailer_bracket n=3 i=0 bt=0
 depth=36                                     trailer n=3 i=2 bt=1
 depth=36                                     trailer_period n=2 i=0 bt=0
-depth=35                                    atom_expr_trailers n=2 i=1 bt=0
+depth=35                                    atom_expr_trailers n=2 i=1 bt=1

I wonder why it backtracks on RISC-V, but I can't look at this any further until tomorrow.

[agatti]

The parser failure was due to port configuration flags that were no longer relevant since when the files were created, and that has been sorted out.

However, updating the makefiles to properly include libm brought up another incompatibility with Picolibc. I've filed #15183, as I thought that was the least intrusive fix to this issue. With that applied the whole lot builds and passes tests on an Ubuntu 22.04 VM.

[agatti]

So here it is in its final form (hopefully). I've also enabled native NLR and GC support as well, following the QEMU Arm port. All tests pass, and my CI integration seems to be working :)

[projectgus]

This looks really great, thanks @agatti! I left a couple of comments but they're about relatively minor things.

I do have a niggling thought about whether the CI should build and test both a newlib variant and a picolibc variant, in case of unexpected regression in one or the other. I'm not sure how likely that kind of bug is though, what do you think?

[agatti]

I do have a niggling thought about whether the CI should build and test both a newlib variant and a picolibc variant, in case of unexpected regression in one or the other. I'm not sure how likely that kind of bug is though, what do you think?

That makes a lot of sense, but I believe that can be added as a separate PR after this is merged. Right now the known incompatibilities between picolibc and newlib on RISC-V are restricted to niche math functions (picolibc's cosh(-∞) is wrong on QEMU but untested on bare metal, and newlib's tgamma(-∞) doesn't return a NaN on my ESP32C3 with IDF 5.0.4 but works fine on QEMU), so it gives us a bit of time to address this at a later stage :)

Ideally, adding a new RV32/newlibc target can be done at the same time as Arm/picolibc (and optionally ESP8266/picolibc)?

[mattytrentini]

This is a really interesting addition to MicroPython. 👍🏻

[mattytrentini]

This appears to be a copy of qemu-arm/test_main.c - which is fine. But perhaps a comment (in both files?) to mention the other in case a change should affect both?

Or maybe finding somewhere common to store it would be worthwhile?

[agatti]

It's not an exact copy, the test heap size is doubled. Or rather, having a common location for common QEMU port files that can be tweaked via #defines in a place like ports/qemu-base/ or shared/qemu/ would be nice, but right now I had to duplicate that file.

[dpgeorge]

Eventually we would like to make some common code to share among the qemu ports, but for now this copy is OK.

[mattytrentini]

(Very minor!) 2024. In a number of files.

[agatti]

The files were created in 2023, so I think they're correct. Otherwise every year every file in the repo should have its copyright notice updated accordingly, which isn't the case.

[dpgeorge]

Yes, if it was written in 2023 then the date can stay as 2023 (it could also be updated to 2023-2024 but that's not critical).

[dpgeorge]

Merged!

I made the following very minor changes during the merge:

• reordered the qemu-riscv additions in tests/run-tests.py so that these additions appear right after the existing qemu-arm bits
• converted tabs to spaces in ports/qemu-riscv/virt.ld

Thank you @agatti for your work on this new port, it's a great addition for testing the RV32 platform.