RFC: Riscv bare metal CI job

TheCharlatan commented at 8:30 am on December 5, 2024: contributor

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 commented at 8:30 am on December 5, 2024: contributor

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, hebasto

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.

TheCharlatan force-pushed on Dec 5, 2024

DrahtBot commented at 9:12 am on December 5, 2024: contributor

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

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.

DrahtBot added the label CI failed on Dec 5, 2024

maflcko commented at 9:16 am on December 5, 2024: member

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 force-pushed on Dec 5, 2024

TheCharlatan commented at 9:26 am on December 5, 2024: contributor

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.

TheCharlatan marked this as ready for review on Dec 5, 2024

build: Add option for building for bare metal envs

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.

dbb1c45ce8

TheCharlatan force-pushed on Dec 5, 2024

in ci/test/03_test_script.sh:168 in 4910ae5d0d outdated

163+        -Wl,--no-whole-archive \
164+        src/crypto/libbitcoin_crypto.a \
165+        src/secp256k1/lib/libsecp256k1.a \
166+        /opt/riscv-ilp32/riscv32-unknown-elf/lib/libstdc++.a \
167+        /riscv/newlib/build/riscv32-unknown-elf/newlib/libc.a \
168+        /riscv/newlib/build/riscv32-unknown-elf/newlib/libm.a \

laanwj commented at 11:20 am on December 5, 2024:

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

in ci/test/01_base_install.sh:98 in 4910ae5d0d outdated

93+
94+    ${CI_RETRY_EXE} git clone --depth=1 https://sourceware.org/git/newlib-cygwin.git -b topic/3.6 /riscv/newlib
95+    cd /riscv/newlib
96+    mkdir build && cd build
97+    ../configure \
98+        --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\

laanwj commented at 11:22 am on December 5, 2024:

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

TheCharlatan commented at 2:25 pm on December 6, 2024:

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 commented at 11:35 am on December 5, 2024: member

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.

laanwj added the label Build system on Dec 5, 2024

laanwj added the label Tests on Dec 5, 2024

DrahtBot removed the label CI failed on Dec 5, 2024

in ci/test/01_base_install.sh:89 in 4910ae5d0d outdated

83@@ -84,6 +84,29 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
84   rm -rf /msan/llvm-project
85 fi
86 
87+if [[ ${BARE_METAL_RISCV} == "true" ]]; then
88+    ${CI_RETRY_EXE} git clone --depth=1 https://github.com/riscv-collab/riscv-gnu-toolchain -b 2024.11.22 /riscv/gcc
89+    cd /riscv/gcc

maflcko commented at 1:35 pm on December 5, 2024:

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

in ci/test/01_base_install.sh:107 in 4910ae5d0d outdated

102+        AR_FOR_TARGET=/opt/riscv-ilp32/bin/riscv32-unknown-elf-ar \
103+        RANLIB_FOR_TARGET=/opt/riscv-ilp32/bin/riscv32-unknown-elf-ranlib \
104+        CFLAGS_FOR_TARGET="-march=rv32gc -mabi=ilp32 -mcmodel=medlow "\
105+        CXXFLAGS_FOR_TARGET="-std=c++20 -march=rv32gc -mabi=ilp32 -mcmodel=medlow"
106+    make -j "$MAKEJOBS"
107+    make install

maflcko commented at 1:38 pm on December 5, 2024:

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

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

See the llvm build on how to do this.

TheCharlatan commented at 2:39 pm on December 6, 2024:

Should be better now:

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

Add CI job for producing a static bare metal binary 662f67bd4a

Add cirrus CI job for riscv bare metal f27760012b

TheCharlatan force-pushed on Dec 6, 2024

TheCharlatan commented at 3:39 pm on December 6, 2024: contributor

Updated 4910ae5d0d9954b8b811d7c52e3539979ea46016 -> f27760012b3e236933ea5d1b39a6ba74884df1a2 (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.

in ci/test/03_test_script.sh:154 in f27760012b

149+              la gp, __global_pointer$
150+              .option pop
151+
152+              call main
153+
154+              # Put Exit2 system call number into the a7 register

laanwj commented at 12:46 pm on December 10, 2024:

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 commented at 12:58 pm on December 10, 2024: member

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.

hebasto commented at 10:53 am on December 14, 2024: member

Concept ACK.

RFC: Riscv bare metal CI job #31425

Code Coverage & Benchmarks

Reviews

Conflicts