Skip to content
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

Add first party C tests using Zig for cross-compilation #759

Closed
wants to merge 1 commit into from

Conversation

Timmmm
Copy link
Collaborator

@Timmmm Timmmm commented Feb 26, 2025

Add first party C tests that can be used for tests specific to this model. This uses Zig as the compiler since it's small and easy to set up. We could automatically download it for users like we can for GMP (I haven't implemented that yet).

This includes a copy of nanoprinf for printing to the UART. It has two tests, test_hello_world.c, which passes, and test_max_pmp.c which fails due to a model bug.

There are some TODOs and a lot of it needs cleaning up a fair but bit I wanted some feedback before I continue.

Fixes #740

Add first party C tests that can be used for tests specific to this model. This uses Zig as the compiler since it's small and easy to set up. We could automatically download it for users like we can for GMP (I haven't implemented that yet).

This includes a copy of `nanoprinf` for printing to the UART. It has two tests, `test_hello_world.c`, which passes, and `test_max_pmp.c` which fails due to a model bug.

There are some TODOs and a lot of it needs cleaning up a fair but bit I wanted some feedback before I continue.
@jrtc27
Copy link
Collaborator

jrtc27 commented Feb 26, 2025

I can't see anything here that justifies using Zig over Clang? You're not even benefiting from its pre-built sysroots. This could easily just be a bunch of Clang invocations generated by CMake itself.

@jordancarlin
Copy link
Collaborator

I have to agree here. I don't see a reason to go with Zig over a standard C compiler. Especially because I assume that most people developing the model will already have a riscv compiler of some sort installed (likely either gcc or clang). While this might not be true for all users (and they won't need it), it seems like a fairy safe assumption for developers.

You also mention potentially running these tests by default only in CI, which makes it seem even less necessary to use something like Zig because we can guarantee a compatible version of Clang/GCC in the CI runner. I think it is more likely that developers will run these tests locally before pushing if they don't need to install a (likely new) compiler first.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Feb 27, 2025

It's true we're not taking advantage of the sysroots, but I still think there are some significant advantages:

  • The download is a couple of orders of magnitude smaller, so it's feasible to do it automatically. I don't think users will want us to download 1.5GB it Clang for them.
  • As a result it's easy to ensure we're using the same compiler version. In theory it shouldn't matter too much but I can definitely imagine CI failures on version X of Clang that pass on version Y. Particularly if we want to test support for cutting edge extensions.
  • We can use Zig's build system which allows e.g. parsing the C code for model configs/ISA strings to run. I haven't implemented that yet but I imagine it's going to be a lot nicer than in CMake.

But... if people are definitely against this then I will change it to use GCC/Clang.

@Timmmm
Copy link
Collaborator Author

Timmmm commented Feb 28, 2025

I opened another PR using Clang instead: #764 As I expected it was a bit painful, but I did get there in the end.

  1. I have Clang 18 installed from Red Hat (dnf install clang on RHEL 8). It wasn't built with RISC-V support. I download a binary LLVM provides instead which has RISC-V support. Kind of a stupid paper cut so I've added a check for this in the CMake.
  2. Nanoprint supports long double (f128) which means it needs a softfloat implementation. I get link errors with clang though:
❯ clang --target=riscv64  test.c -o test.elf -ffreestanding -nostdlib
clang: /lib64/libtinfo.so.5: no version information available (required by clang)
/home/me/local/clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/clang-18: /lib64/libtinfo.so.5: no version information available (required by /home/me/local/clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/clang-18)
/home/me/local/clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/ld.lld: /lib64/libtinfo.so.5: no version information available (required by /home/codasip.com/me/clang+llvm-18.1.8-x86_64-linux-gnu-ubuntu-18.04/bin/ld.lld)
ld.lld: error: undefined symbol: __divtf3
>>> referenced by test.c
>>>               /tmp/test-27a12e.o:(div)
clang: error: ld.lld command failed with exit code 1 (use -v to see invocation)

I guess there's some f128 softfloat implementation somewhere in Clang, but... again this is just taken care of by Zig.

I just commented out the long double printing code for now.

@jordancarlin
Copy link
Collaborator

Closing now that the pure C version is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add first-party tests
3 participants