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 Report

All modified and coverable lines are covered by tests ✅

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

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

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.

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 @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!

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.

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

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

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

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.

Those PRs have now been merged.

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:

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.

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?

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

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 :)

PR for sorted test generation sent: #12906.

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?

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.

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.

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?

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.

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.

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 :)

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)?

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.