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

cmake: kconfig: llvm: riscv: Add baseline clang/llvm support for RISC-V #68259

Merged

Conversation

jonathonpenix
Copy link
Contributor

Overview

This patch aims to establish a baseline level of support for building with clang/LLVM when targeting RISC-V. Previously, a few minor things were missing that prevented building:

  1. Clang's --target flag and the RISC-V specific -march/-mabi/-mcmodel flags were not being set appropriately, giving unexpected build failures depending on the toolchain's default values for these flags.
  2. TLS support was not "advertised" when building with Clang/LLVM, preventing building tests/apps that rely on this support.

With these two things resolved, a decent number of the tests that support the qemu_riscv32/64 platforms are passing. However, there are still some lingering failures and misc. warnings that I hope to resolve in subsequent PRs--see the "Testing" and "Follow-up work" sections for details. I left these as future work given the number of different issues, but I can slowly work on them and include the fixes here if that would be preferable!

Testing

This patch was tested by enabling 'llvm' in the qemu_riscv32.yaml and qemu_riscv64.yaml files and running the tests for these boards with clang + lld as well as clang + GNU ld. I am using a version of clang/llvm built at llvm/llvm-project@f7669ba (from 1/23/2024, so fairly recent).

Tests were run with the following commands. Please note that -W was specified as there are some unexpected warnings appearing currently.

  • clang + GNU ld command: west twister -W -p qemu_riscv[32/64] -x=CONFIG_LLVM_USE_LD=y -x=CONFIG_COMPILER_RT_RTLIB=y
  • clang + lld command: west twister -W -p qemu_riscv[32/64] -x=CONFIG_LLVM_USE_LLD=y -x=CONFIG_COMPILER_RT_RTLIB=y

Test summaries are shown below:

  • clang + GNU ld, riscv32: INFO - 469 of 2463 test configurations passed (95.71%), 4 failed, 17 errored, 1973 skipped with 0 warnings in 1673.90 seconds
  • clang + GNU ld, riscv64: INFO - 471 of 2463 test configurations passed (96.52%), 12 failed, 5 errored, 1975 skipped with 0 warnings in 1685.06 seconds
  • clang + lld, riscv32: INFO - 469 of 2463 test configurations passed (95.71%), 4 failed, 17 errored, 1973 skipped with 0 warnings in 1740.49 seconds
  • clang + lld, riscv64: INFO - 325 of 2463 test configurations passed (66.60%), 1 failed, 162 errored, 1975 skipped with 0 warnings in 1391.14 seconds

Notable test failures that I'm aware of:

  • lld fails for ~150 tests with relocation out of range errors for riscv64. The values of __tbss_align (for example) are technically out of range from where they are referenced given that we're building with -mcmodel=medany. GNU ld will "relax" the auipc + addi pair to an lui + addi pair, allowing this to work but lld does not implement this. There is some relevant discussion at [lld] [risc-v] absolute values with pcrel relocation cause relocation overflows llvm/llvm-project#78346 (lld) and Document position on zero-page optimization for linker in psABI riscv-non-isa/riscv-elf-psabi-doc#197 (RISC-V psABI). I'm still trying to understand how this should be resolved in the long-term.
  • Some tests (ex: samples/application_development/external_lib/sample.app_dev.external_lib) fail with an error along the lines of unsupported argument 'medany' to option ... for target <default target>. I think the build commands for the external library in the test are not setting --target appropriately.
  • A few tests (ex: tests/arch/riscv/fpu_sharing/arch.riscv.fpu_sharing) fail with error: .option pop with no .option push. This is an issue with clang--it does not handle having .push and .pop specified in separate inline-asm statements well.
  • tests/kernel/workq/work_queue/kernel.workqueue fails with a failed assert &initialized_by_function not equal to &initialized_by_macro. I think this might be an issue with clang--I'm still investigating this though.
  • ~11 runtime failures that seem specific to clang and show up as an unexpected coredump or access fault. A few sample tests where I'm seeing this are tests/kernel/threads/thread_apis/kernel.threads.apis, tests/kernel/common/kernel.common.minimallibc, and tests/subsys/debug/coredump/debug.coredump.logging_backend. I haven't investigated this yet.
  • A few tests (ex: tests/drivers/console_switching/drivers.console_switching) fail to build with errors along the lines of error: unknown type name 'ssize_t'; did you mean 'size_t'?. My understanding is that for these tests Zephyr's toolchain is invoked with --specs=picolibc.specs which helps to find the right definition of ssize_t. Clang doesn't support --specs, so I need to investigate to see how best to support these cases.

Follow-up work

In addition to the test failures above, there are a few other issues with the Zephyr/LLVM/RISC-V combo I'm aware of that I'd like to resolve in later PRs:

  • Pass -m[no-]-global-merge only for Arm/AArch64 targets when building with clang (the flag only has an effect on Arm/AArch64 currently, all other targets see a warning about an unused argument).
  • Constant conversion warning (I'm not sure if this is a clang or Zephyr issue): /workdir/zephyr/drivers/timer/riscv_machine_timer.c:187:47: warning: implicit conversion from 'unsigned long' to 'int32_t' (aka 'int') changes value from 92233720368547 to -702313053 [-Wconstant-conversion]
  • Resolve 'register' warning: /workdir/zephyr/include/zephyr/arch/riscv/arch_inlines.h:17:9: warning: 'register' storage class specifier is deprecated and incompatible with C++17
  • lld gives a few misc. warnings for .symtab, .strtab etc.
  • lld needs --relax-gp to be explicitly passed for gp-relaxation to be enabled.

Questions

  1. One consequence of using include(${ZEPHYR_BASE}/cmake/compiler/gcc/target_riscv.cmake) in cmake/compiler/clang/target.cmake is that it requires a version of LLVM that is able to handle the separate zicsr and zifencei extensions. This was only very recently implemented in LLVM, however (llvm/llvm-project@22e199e). So, effectively this would require an LLVM version >= 17.x. Would it be better to version gate adding zicsr and zifencei to the -march string for incompatible LLVM versions? I think doing so should be correct from a RISC-V/LLVM standpoint, but I'm not sure what makes sense from Zephyr's standpoint.
  2. Would it make sense to enable 'llvm' in the qemu_riscv[32/64].yaml config files to make testing easier for others? I didn't mark llvm as supported due to the number of failures, but I'd like to enable this long-term if that would be ok.

Ensure --target and -march/-mabi/-mcmodel are set appropriately when
building with clang targeting RISC-V.

Signed-off-by: Jonathon Penix <jpenix@quicinc.com>
clang and lld support TLS for RISC-V, so advertise this support in
Kconfig. I believe other non-RISC-V targets also have TLS support in LLVM,
but I'm not sure on the exact subset. As TLS support in LLVM wasn't
automatically advertised previously, gate this on RISC-V for now as it
a) shouldn't break other targets and b) prevents us from improperly
claiming support for unsupported targets.

Signed-off-by: Jonathon Penix <jpenix@quicinc.com>
Copy link

Hello @jonathonpenix, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@jonathonpenix
Copy link
Contributor Author

Gentle ping on this.

(I dropped this for a bit but I'm hoping to pick the LLVM/RISC-V/Zephyr work back up. Any advice on the questions above and whether all the LLVM/RISC-V/Zephyr failures described above need to be resolved prior to merging, etc. would be greatly appreciated!)

@nordicjm nordicjm requested a review from fabiobaltieri March 18, 2024 11:30
@aescolar aescolar merged commit 82a47cc into zephyrproject-rtos:main Mar 18, 2024
24 checks passed
@jonathonpenix jonathonpenix deleted the basic_clang_riscv_support branch March 18, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants