RFC: Riscv bare metal CI job #31425

TheCharlatan · 2024-12-05T08:30:01Z

This adds a CI job for building the static consensus library and linking it to an executable. It uses newlib-cygwin as a C library for the final linking step. This ensure compatibility with this target going forward and can serve as a starting point for enabling bare metal builds for the entire kernel library. This would have also caught the error fixed in #31365.

DrahtBot · 2024-12-05T08:30:04Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31425.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31394 ([POC] cmake: Introduce LLVM's Source-based Code Coverage reports by hebasto)
#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2024-12-05T09:12:36Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/33960779294}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko · 2024-12-05T09:16:04Z

can serve as a starting point for enabling bare metal builds for the entire kernel library.

Interesting. Do you think this is possible at all, given that the kernel library links leveldb, which I'd presume is not bare-metal ready?

So I guess this mostly serves as a check that users can ship their own-brewed libbitcoinconsensus (or so, or subset of it)?

TheCharlatan · 2024-12-05T09:26:27Z

Interesting. Do you think this is possible at all, given that the kernel library links leveldb, which I'd presume is not bare-metal ready?

Yes, definitely not ready. I think the main hurdle is the background compaction, which I am not sure how to tackle. Maybe we'll find a solution for it eventually though, either by patching it, or allowing the user to bring their own utxos.

So I guess this mostly serves as a check that users can ship their own-brewed libbitcoinconsensus (or so, or subset of it)?

Yes, that is the goal for now.

A bare metal build is now supported by setting CMAKE_SYSTEM_NAME=Generic Skip the platform-dependent feature checks, such as threads and atomics, which are typically not available on bare metal. Also only make the boost headers mandatory if they exist for the target.

laanwj · 2024-12-05T11:20:01Z

ci/test/03_test_script.sh

+        src/secp256k1/lib/libsecp256k1.a \
+        /opt/riscv-ilp32/riscv32-unknown-elf/lib/libstdc++.a \
+        /riscv/newlib/build/riscv32-unknown-elf/newlib/libc.a \
+        /riscv/newlib/build/riscv32-unknown-elf/newlib/libm.a \


Any specific reason to use the intermediate build and not the installed libraries from /opt/newlib, here?

laanwj · 2024-12-05T11:22:45Z

ci/test/01_base_install.sh

+    cd /riscv/newlib
+    mkdir build && cd build
+    ../configure \
+        --target=riscv32-unknown-elf --disable-newlib-io-float --enable-newlib-io-long-long --enable-newlib-io-long-double --with-arch=rv32gc --with-abi=ilp32 --disable-shared --disable-multilib\


Are you sure enabling i/o for long-double is needed? i don't believe we use this type anywhere.

Sorry, all these flags were a mess. I was experimenting with linking in a range of other functionality, as well as running it on linux directly, and didn't prune stuff out nicely. Removed most of them again.

laanwj · 2024-12-05T11:35:52Z

It uses newlib-cygwin as a C library for the final linking step.

Mentioning this because i had to look it up to be sure: newlib-cygwin has nothing to do with Windows whatsoever. It's simply a minimalist libc.

I think the main hurdle is the background compaction, which I am not sure how to tackle.

Could be a periodic foreground task, if threads aren't available? But yes, this would imply patching leveldb, there is no such API right now.

maflcko · 2024-12-05T13:35:28Z

ci/test/01_base_install.sh

@@ -84,6 +84,29 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
  rm -rf /msan/llvm-project
 fi

+if [[ ${BARE_METAL_RISCV} == "true" ]]; then
+    ${CI_RETRY_EXE} git clone --depth=1 https://github.com/riscv-collab/riscv-gnu-toolchain -b 2024.11.22 /riscv/gcc
+    cd /riscv/gcc


style-wise, it would be good to not use cd in the CI scripts, because it affects the global state for all remaining lines.

It is better to wrap it into (), for example ( cd a ; foo(); ) (or similar).

maflcko · 2024-12-05T13:38:00Z

ci/test/01_base_install.sh

+        CFLAGS_FOR_TARGET="-march=rv32gc -mabi=ilp32 -mcmodel=medlow "\
+        CXXFLAGS_FOR_TARGET="-std=c++20 -march=rv32gc -mabi=ilp32 -mcmodel=medlow"
+    make -j "$MAKEJOBS"
+    make install


It would be good to rm -rf everything duplicate. Otherwise, you are using up space twice:

$ podman image ls | grep riscv localhost/ci_native_riscv_bare latest dfa9b8223b62 5 minutes ago 13.9 GB

See the llvm build on how to do this.

Should be better now:

docker image ls | grep riscv ci_native_riscv_bare latest 55627eefe6d7 3 minutes ago 2.8GB

TheCharlatan · 2024-12-06T15:39:07Z

Updated 4910ae5 -> f277600 (bare_metal_support_0 -> bare_metal_support_1, compare)

Addressed @maflcko's comment, removing sources after install step.
Addressed @maflcko's comment, wrapped cd step into a subshell
Addressed @laanwj's comment, use the installed lib
Addressed @laanwj's comment, removing a bunch of unneeded/superfluous flags
Added a start section to the binary for the global pointer and returning an exit code. Though not needed, since we are only checking that it links, I feel like this makes the example a bit clearer.

laanwj · 2024-12-10T12:46:12Z

ci/test/03_test_script.sh

+
+              call main
+
+              # Put Exit2 system call number into the a7 register


concept ACK on making it actually runnable somewhat
i was confused for a moment here, as to what syscall numbers mean for bare-metal
maybe add "Linux" to the comment

laanwj · 2024-12-10T12:58:09Z

Concept ACK, this looks like the minimum required to sanity check that libconsensus.a can be compiled for, and linked for bare-metal RISC-V.
It is not enough to test that it actually works (this would require a lot more, quite nasty low-level code), but maybe that's out of scope for this project. At the least it is for this PR.

TheCharlatan force-pushed the bare_metal_support branch from e75c002 to ece9dcd Compare December 5, 2024 09:12

DrahtBot added the CI failed label Dec 5, 2024

TheCharlatan force-pushed the bare_metal_support branch from ece9dcd to bc5adc3 Compare December 5, 2024 09:22

TheCharlatan marked this pull request as ready for review December 5, 2024 10:05

TheCharlatan force-pushed the bare_metal_support branch from bc5adc3 to 4910ae5 Compare December 5, 2024 10:33

laanwj reviewed Dec 5, 2024

View reviewed changes

laanwj added Build system Tests labels Dec 5, 2024

DrahtBot removed the CI failed label Dec 5, 2024

DrahtBot mentioned this pull request Dec 5, 2024

[POC] cmake: Introduce LLVM's Source-based Code Coverage reports #31394

Draft

maflcko reviewed Dec 5, 2024

View reviewed changes

DrahtBot mentioned this pull request Dec 5, 2024

ci: Test cross-built Windows executables on Windows natively #31176

Open

TheCharlatan added 2 commits December 6, 2024 15:58

Add CI job for producing a static bare metal binary

662f67b

Add cirrus CI job for riscv bare metal

f277600

TheCharlatan force-pushed the bare_metal_support branch from 4910ae5 to f277600 Compare December 6, 2024 15:39

laanwj reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Riscv bare metal CI job #31425

RFC: Riscv bare metal CI job #31425

TheCharlatan commented Dec 5, 2024

DrahtBot commented Dec 5, 2024 •

edited

Loading

DrahtBot commented Dec 5, 2024

maflcko commented Dec 5, 2024

TheCharlatan commented Dec 5, 2024

laanwj Dec 5, 2024

laanwj Dec 5, 2024

TheCharlatan Dec 6, 2024

laanwj commented Dec 5, 2024

maflcko Dec 5, 2024

maflcko Dec 5, 2024

TheCharlatan Dec 6, 2024

TheCharlatan commented Dec 6, 2024

laanwj Dec 10, 2024 •

edited

Loading

laanwj commented Dec 10, 2024 •

edited

Loading


		call main

		# Put Exit2 system call number into the a7 register

RFC: Riscv bare metal CI job #31425

Are you sure you want to change the base?

RFC: Riscv bare metal CI job #31425

Conversation

TheCharlatan commented Dec 5, 2024

DrahtBot commented Dec 5, 2024 • edited Loading

Code Coverage & Benchmarks

Reviews

Conflicts

DrahtBot commented Dec 5, 2024

maflcko commented Dec 5, 2024

TheCharlatan commented Dec 5, 2024

laanwj Dec 5, 2024

Choose a reason for hiding this comment

laanwj Dec 5, 2024

Choose a reason for hiding this comment

TheCharlatan Dec 6, 2024

Choose a reason for hiding this comment

laanwj commented Dec 5, 2024

maflcko Dec 5, 2024

Choose a reason for hiding this comment

maflcko Dec 5, 2024

Choose a reason for hiding this comment

TheCharlatan Dec 6, 2024

Choose a reason for hiding this comment

TheCharlatan commented Dec 6, 2024

laanwj Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

laanwj commented Dec 10, 2024 • edited Loading

DrahtBot commented Dec 5, 2024 •

edited

Loading

laanwj Dec 10, 2024 •

edited

Loading

laanwj commented Dec 10, 2024 •

edited

Loading