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

build: fix linking against -latomic when building for riscv #20938

Merged
merged 2 commits into from
Jan 18, 2021

Conversation

fanquake
Copy link
Member

Since the merge of #19937, riscv builds have been failing, due to a link issue with std::atomic_exchange in bitcoin-util:

  CXXLD    bitcoin-util
bitcoin_util-bitcoin-util.o: In function `grind_task':
/home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
collect2: error: ld returned 1 exit status

We have a macro that tries to determine when -latomic is required, however it doesn't quite work well enough, as it's currently determining it isn't needed:

./autogen.sh
./configure --prefix=/home/ubuntu/bitcoin/depends/riscv64-linux-gnu
...
checking whether std::atomic can be used without link library... yes

This PR adds a call to std::atomic_exchange to the macro, which will get us properly linked against -latomic on riscv:

checking whether std::atomic can be used without link library... no
checking whether std::atomic needs -latomic... yes

Also adds an <atomic> include to bitcoin-util.cpp.

riscv builds are currently failing because -latomic isn't being linked
against, when it is needed:
```bash
/home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
```

This exteneds our macro to ensure that -latomic is linked against when
required.
@fanquake fanquake changed the title Fix linking against -latomic when building for riscv build: fix linking against -latomic when building for riscv Jan 15, 2021
@laanwj
Copy link
Member

laanwj commented Jan 18, 2021

Testing this. Doing a compile on the SiFive Unleashed is a bit slow, so give it some time.

Master without this PR:

configure:5619: checking whether std::atomic can be used without link library
configure:5637: g++ -std=c++17 -o conftest -g -O2   conftest.cpp  >&5
configure:5637: $? = 0
configure:5639: result: yes

Now trying a build of master to see if it fails. Will then try with this PR.

Edit: this clang issue seems related: https://reviews.llvm.org/D69869 . The conclusion is the same—RISC-V GCC needs libatomic for some atomic operations, but not others.

Edit2: i got some awful, unrelated DWARF error during linking, i think my build toolchain is messed up

Edit3: reproduced

/usr/bin/ld: bitcoin_util-bitcoin-util.o: in function `.L0 ':
/data/src/bitcoin/src/bitcoin-util.cpp:102: undefined reference to `__atomic_exchange_1'
collect2: error: ld returned 1 exit status
gmake[2]: *** [Makefile:5562: bitcoin-util] Error 1
gmake[2]: *** Waiting for unfinished jobs....

@laanwj
Copy link
Member

laanwj commented Jan 18, 2021

Tested ACK 54ce4fa

@laanwj laanwj merged commit c763cac into bitcoin:master Jan 18, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 20, 2021
…g for riscv

54ce4fa build: improve macro for testing -latomic requirement (fanquake)
2c010b9 add std::atomic include to bitcoin-util.cpp (fanquake)

Pull request description:

  Since the merge of bitcoin#19937, riscv builds have been failing, due to a link issue with [`std::atomic_exchange`](https://en.cppreference.com/w/cpp/atomic/atomic_exchange) in `bitcoin-util`:
  ```bash
    CXXLD    bitcoin-util
  bitcoin_util-bitcoin-util.o: In function `grind_task':
  /home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
  collect2: error: ld returned 1 exit status

  ```

  We have a [macro](https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/l_atomic.m4) that tries to determine when `-latomic` is required, however it doesn't quite work well enough, as it's currently determining it isn't needed:
  ```bash
  ./autogen.sh
  ./configure --prefix=/home/ubuntu/bitcoin/depends/riscv64-linux-gnu
  ...
  checking whether std::atomic can be used without link library... yes
  ```

  This PR adds a call to `std::atomic_exchange` to the macro, which will get us properly linked against `-latomic` on riscv:
  ```bash
  checking whether std::atomic can be used without link library... no
  checking whether std::atomic needs -latomic... yes
  ```

  Also adds an `<atomic>` include to `bitcoin-util.cpp`.

ACKs for top commit:
  laanwj:
    Tested ACK 54ce4fa

Tree-SHA512: 963c875097ee96b131163ae8109bcf8fecf4451d20faa2f3d223f9938ea3d8d1ed5604e12ad82c2b4b1c605fd293a9b6b08fefc00dd3e68d09c49e95029c6f50
@fanquake fanquake deleted the fix_riscv_atomic_usage branch February 6, 2021 06:01
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
riscv builds are currently failing because -latomic isn't being linked
against, when it is needed:
```bash
/home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1'
```

This exteneds our macro to ensure that -latomic is linked against when
required.

Github-Pull: bitcoin#20938
Rebased-From: 54ce4fa
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jan 9, 2022
c76e7b6 [Core] Fix data races in bls_worker and use ctpl_stl queue (Fuzzbawls)
55babf4 [Build] Improve std::atomic test macro (Fuzzbawls)

Pull request description:

  Coming from dashpay#4240, bitcoin#20938, and bitcoin#21920, this resolves numerous compile-time inconsistencies/failures on certain combos of gcc and cpu arch that were observed on launchpad.net over the 5.4.0 release cycle.

ACKs for top commit:
  furszy:
    ACK c76e7b6
  random-zebra:
    ACK c76e7b6

Tree-SHA512: d931142f978d0d22668b9ff2e16d48c148b7d163b35aeef149ab08a13174536c035f2581a8ec5bc9782226fcb27ed33b08aec475d1522d8e8532d00a12c5df72
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants