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

depends: Add RISC-V support #13543

Merged
merged 5 commits into from Jul 10, 2018

Conversation

Projects
None yet
6 participants
@laanwj
Copy link
Member

commented Jun 27, 2018

This adds support for riscv32 and riscv64 builds to the depends system.

The change consists of documentation and build system changes. The most significant change is an update of config.sub and config.guess inside zeromq patch, as the current version does not recognize the riscv* host tuples (there's no new version of ZeroMQ yet with newer ones).

Good thing: RISC-V 64-bit toolchain packages can be installed out of the box on Ubuntu 18.04+.

I would also like to add RISC-V 64-bit executables to gitian, but this will not be possible until #12511 .

@fanquake

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@laanwj Have you been building with NO_QT=1?

I've just tested a make HOST=riscv64-linux-gnu depends build on Ubuntu 18.04, after installing the RISC-V toolchain and am seeing:

Configuring qrencode...
checking build system type... x86_64-unknown-linux-gnu
checking host system type... Invalid configuration `riscv64-linux-gnu': machine `riscv64' not recognized
configure: error: /bin/bash use/config.sub riscv64-linux-gnu failed

Building with NO_QT=1 completes successfully.

I wonder if there's a less verbose patch we could et away with to fix the zeromq build?

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Thanks for testing. Good point: yes, I have built without Qt, same as we do for ARM. Should probably mention that in the documentation, though I don't think there's any RISC-V boards at this time with a capable enough GPU and HDMI/VGA out to run even a desktop GUI (though maybe it's possible to connect one through the PCI-e slot? I don't know).

I wonder if there's a less verbose patch we could et away with to fix the zeromq build?

I'm sure it could be cut down to less lines if that's really worthwhile, I might personally prefer the dumb solution of simply copying the new autoconf files though.

@tmagik

This comment has been minimized.

Copy link

commented Jun 27, 2018

I can test this on a HiFiveU board next week. The problem with running a full bitcoin main net instance on a physical riscv board is going to be I/O.

In the meantime, try running in Qemu as it will probably have better I/O

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

I can test this on a HiFiveU board next week. The problem with running a full bitcoin main net instance on a physical riscv board is going to be I/O.

Running test_bitcoin would already be helpful, even if you don't want to sync a chain.

In the meantime, try running in Qemu as it will probably have better I/O

Good idea. I did this for the big-endian support, so it should certainly work for little-endian RISC-V.

@tmagik

This comment has been minimized.

Copy link

commented Jun 27, 2018

Thanks for testing. Good point: yes, I have built without Qt, same as we do for ARM. Should probably mention that in the documentation, though I don't think there's any RISC-V boards at this time with a capable enough GPU and HDMI/VGA out to run even a desktop GUI (though maybe it's possible to connect one through the PCI-e slot? I don't know).

https://www.crowdsupply.com/microsemi/hifive-unleashed-expansion-board

It runs X with a radeon graphics card and has more cores/memory/performance than the first machine I compiled bitcoin on. Bitcoin ATM vendors looking at high volume, low-cost embedded systems should probably be spending money on some evaluation systems.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

This is interesting:

user@bionic:~/bitcoin/src$ QEMU_LD_PREFIX=/usr/riscv64-linux-gnu qemu-riscv64 ./test/test_bitcoin
Running 291 test cases...
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed
test/crypto_tests.cpp(561): error: in "crypto_tests/sha256d64": check memcmp(out1, out2, 32 * i) == 0 has failed

*** 32 failures are detected in the test module "Bitcoin Test Suite"

It runs X with a radeon graphics card and has more cores/memory/performance than the first machine I compiled bitcoin on. Bitcoin ATM vendors looking at high volume, low-cost embedded systems should probably be spending money on some evaluation systems.

Nice. I should get that one too.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@laanwj That's very strange. It should be using the pure C++ SHA256 implementation. What SHA256 implementation does debug.log report (if you get that far)?

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

@laanwj That's very strange. It should be using the pure C++ SHA256 implementation. What SHA256 implementation does debug.log report (if you get that far)?

It chooses the correct one:

2018-06-27T19:16:24Z Using the 'standard' SHA256 implementation

I'll dive into what happens here.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 28, 2018

It looks like the beginning of input in[] to SHA256D64 becomes corrupted by

        for (int j = 0; j < i; ++j) {
            CHash256().Write(in + 64 * j, 64).Finalize(out1 + 32 * j);
        }

The function itself works correctly.

I'm suspecting a miscompilation issue at the moment. It happens with -O1 and higher but not -O0. I'm not able to narrow it down to a specific optimization option, however it looks like it has something to do with computing stack offsets for inlining.

I'd expect a newer toolchain will solve this, but if so it means the Ubuntu stock one (Ubuntu 7.3.0-16ubuntu3) is effectively broken that's a disappointment.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

My sync in qemu is at block 270000, and has encountered no issues. I haven't figured out yet why the crypto_tests/sha256d64 specifically has this issue. Strangely enough, moving the in array to the outer scope makes the test pass.
I intend to try compilng with a more recent version of gcc to see if the test problem is resolved.

@tmagik

This comment has been minimized.

Copy link

commented Jun 30, 2018

I attempted to make a test case, and simplified the following down:

static inline uint64_t InsecureRandBits(int bits) { return insecure_seed++; }

however this does not fail, so I'm now highly suspicious of inlining

tmagik added a commit to tmagik/riscv-sha256d-inline-fail that referenced this pull request Jul 1, 2018

@tmagik

This comment has been minimized.

Copy link

commented Jul 1, 2018

I can confirm the failure with gcc-7.3.1 on fedora on a HiFive Unleashed as well as gcc-7.3.0 on debian in qemu-user-mode emulation.

Testcase at https://github.com/tmagik/riscv-sha256d-inline-fail/

@tmagik

This comment has been minimized.

Copy link

commented Jul 1, 2018

@laanwj Debian's gcc-8 appears to fix the problem

riscv64-linux-gnu-gcc-8 --version
riscv64-linux-gnu-gcc-8 (Debian 8.1.0-8) 8.1.0
@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2018

@tmagic Thanks for reproducing. Happy to hear it's solved with newer gcc. It might be better to remove the note about using ubuntu's cross-toolchain packages, then, until they have gcc 8 like debian.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13578 ([depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions by mruddy)

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.

@theuni

theuni approved these changes Jul 6, 2018

Copy link
Member

left a comment

utACK, suggested changes would be a bonus if they work.

depends/packages/zeromq.mk Outdated
@@ -3,7 +3,7 @@ $(package)_version=4.2.3
$(package)_download_path=https://github.com/zeromq/libzmq/releases/download/v$($(package)_version)/
$(package)_file_name=$(package)-$($(package)_version).tar.gz
$(package)_sha256_hash=8f1e2b2aade4dbfde98d82366d61baef2f62e812530160d2e6d0a5bb24e40bc0
$(package)_patches=0001-fix-build-with-older-mingw64.patch 0002-disable-pthread_set_name_np.patch
$(package)_patches=0001-fix-build-with-older-mingw64.patch 0002-disable-pthread_set_name_np.patch 0003-new_configure.patch

This comment has been minimized.

Copy link
@theuni

theuni Jul 6, 2018

Member

Could we copy our files instead, since we have to keep ours up to date anyway?

This comment has been minimized.

Copy link
@fanquake

fanquake Jul 8, 2018

Member

I've added some commits to a branch that attempt to do that: https://github.com/fanquake/bitcoin/tree/riscv-depends-no-zmq-patching. It also updates the depends config.guess/sub to latest upstream.

@laanwj You can cherry pick from there if you'd like. Note that branch doesn't include re-running ./autogen.sh like below. I'll comment about that shortly.

depends/packages/zeromq.mk Outdated
patch -p1 < $($(package)_patch_dir)/0002-disable-pthread_set_name_np.patch
patch -p1 < $($(package)_patch_dir)/0002-disable-pthread_set_name_np.patch && \
patch -p1 < $($(package)_patch_dir)/0003-new_configure.patch && \
./autogen.sh

This comment has been minimized.

Copy link
@theuni

theuni Jul 6, 2018

Member

Is it definitely necessary to re-run autogen.sh? Afaik configure just calls out to config.guess/config.sub. Unless there's some weird globbing involved, I should think that replacing those files would be enough.

fanquake added some commits Jul 8, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 8, 2018

I've done some more testing of this using a branch that doesn't patch zmq's config.guess/sub, and instead just copies over our local versions, which I've updated from upstream.

re running ./autogen.sh after copying.
With or without './autogen.sh I see this warning from the zmq build:

Staging zeromq...
5.	make[1]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.	5.	make[1]: warning: jobserver unavailable: using -j1.  Add '+' to parent make rule.

The other difference (at least in output) is:
With ./autogen.sh:

libtool: warning: remember to run 'libtool --finish /home/ubuntu/bitcoin/depends/riscv64-linux-gnu/lib'

Without ./autogen.sh:

libtool: install: warning: remember to run `libtool --finish /home/ubuntu/bitcoin/depends/riscv64-linux-gnu/lib'

Also noticed that when I pass --enable-werror to configure I see:

./configure --prefix=`pwd`/depends/riscv64-linux-gnu --enable-werror
<snip>

  debug enabled = no
  gprof enabled = no
  werror        = yes

  target os     = linux
  build os      = 

  CC            = riscv64-linux-gnu-gcc
  CFLAGS        = -pipe -O2 
  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I/home/ubuntu/bitcoin/depends/riscv64-linux-gnu/share/../include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = riscv64-linux-gnu-g++ -std=c++11
  CXXFLAGS      =   -Wstack-protector -fstack-protector-all    -Werror=vla  -pipe -O2 
  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie  -L/home/ubuntu/bitcoin/depends/riscv64-linux-gnu/share/../lib 
  ARFLAGS       = cr
  CXXLD    bench/bench_bitcoin
  CXXLD    test/test_bitcoin
/home/ubuntu/bitcoin/depends/riscv64-linux-gnu/share/../lib/libboost_unit_test_framework-mt.a(execution_monitor.o): In function `.L0 ':
execution_monitor.cpp:(.text+0x9a4): warning: fedisableexcept is not implemented and will always fail
execution_monitor.cpp:(.text+0x96a): warning: feenableexcept is not implemented and will always fail
make[2]: Leaving directory '/home/ubuntu/bitcoin/src'

All my testing has been done on Ubuntu 18.04.

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

@theuni thanks for review
@fanquake thanks for making the changes - will take over your branch

@laanwj laanwj force-pushed the laanwj:2018_06_riscv_depends branch to 974f0bf Jul 9, 2018

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

Updated branch:

  • Took over @fanquake's branch that copies the autoconf artifacts instead of the big ugly patch
  • Added a comment about the RISC-V compilation issue
@theuni

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

utACK 974f0bf.

@laanwj laanwj merged commit 974f0bf into bitcoin:master Jul 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 10, 2018

Merge #13543: depends: Add RISC-V support
974f0bf depends: Mention RISC-V known compilation issue with gcc-7.3.x (Wladimir J. van der Laan)
0d1f38c depends: update zmq config.guess/config.sub for riscv support (fanquake)
409481c depends: latest config.sub (fanquake)
d7005e9 depends: latest config.guess (fanquake)
359e2e3 depends: Add RISC-V support (Wladimir J. van der Laan)

Pull request description:

  This adds support for riscv32 and riscv64 builds to the depends system.

  The change consists of documentation and build system changes. The most significant change is an update of `config.sub` and `config.guess` inside zeromq patch, as the current version does not recognize the `riscv*` host tuples (there's no new version of ZeroMQ yet with newer ones).

  Good thing: RISC-V 64-bit toolchain packages can be installed out of the box on Ubuntu 18.04+.

  I would also like to add RISC-V 64-bit executables to gitian, but this will not be possible until #12511 .

Tree-SHA512: 358ed72ee9e4ae44e7d305c09a4ff5ce5460eeb7ed915eb25d39c8f43b61e7b347f51bf0ae5d83ddb4ce8876dea7703c926b3baa3cccb4932b3bc17160d801bb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.