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

Improve build detect for RISCV #9366

Closed
wants to merge 2 commits into from

Conversation

adamretter
Copy link
Collaborator

@adamretter adamretter commented Jan 7, 2022

Related to: #9215

  • Adds build_detect_platform support for RISCV on Linux (at least on SiFive Unmatched platforms)

This still leaves some linking issues on RISCV remaining (e.g. when building db_test):

/usr/bin/ld: ./librocksdb_debug.a(memtable.o): in function `__gnu_cxx::new_allocator<char>::deallocate(char*, unsigned long)':
/usr/include/c++/10/ext/new_allocator.h:133: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: ./librocksdb_debug.a(memtable.o): in function `std::__atomic_base<bool>::compare_exchange_weak(bool&, bool, std::memory_order, std::memory_order)':
/usr/include/c++/10/bits/atomic_base.h:464: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: /usr/include/c++/10/bits/atomic_base.h:464: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: /usr/include/c++/10/bits/atomic_base.h:464: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: /usr/include/c++/10/bits/atomic_base.h:464: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: ./librocksdb_debug.a(memtable.o):/usr/include/c++/10/bits/atomic_base.h:464: more undefined references to `__atomic_compare_exchange_1' follow
/usr/bin/ld: ./librocksdb_debug.a(db_impl.o): in function `rocksdb::DBImpl::NewIteratorImpl(rocksdb::ReadOptions const&, rocksdb::ColumnFamilyData*, unsigned long, rocksdb::ReadCallback*, bool, bool)':
/home/adamretter/rocksdb/db/db_impl/db_impl.cc:3019: undefined reference to `__atomic_exchange_1'
/usr/bin/ld: ./librocksdb_debug.a(write_thread.o): in function `rocksdb::WriteThread::Writer::CreateMutex()':
/home/adamretter/rocksdb/./db/write_thread.h:205: undefined reference to `__atomic_compare_exchange_1'
/usr/bin/ld: ./librocksdb_debug.a(write_thread.o): in function `rocksdb::WriteThread::SetState(rocksdb::WriteThread::Writer*, unsigned char)':
/home/adamretter/rocksdb/db/write_thread.cc:222: undefined reference to `__atomic_compare_exchange_1'
collect2: error: ld returned 1 exit status
make: *** [Makefile:1449: db_test] Error 1

@adamretter adamretter added RiscV64 Build or run RocksDB on Risc s390x and removed CLA Signed labels Jan 7, 2022
@adamretter
Copy link
Collaborator Author

This looks like it might help - advancedtelematic/aktualizr#1427

@jonathan-albrecht-ibm
Copy link
Contributor

Hi @adamretter, I might be misunderstanding something but is the change in cfeee6 what we want on s390x if PORTABLE=1? IIUC, if -march=native and the lib is built on say a z15 machine the lib could end up not being able to be run on an earlier generation machine since we're telling gcc its ok to generate z15 specific instructions.

I'm thinking mostly about the rocksdbjni build where I think we want to build with -march=z196 to make sure the jar includes a lib that can run on a machine that old or newer. When I put in the change that set -march=z196 when PORTABLE=1 I was thinking it was the way to build the rocksdbjni jar so it could run on z196 or newer, is that being done somewhere else?

@adamretter
Copy link
Collaborator Author

@jonathan-albrecht-ibm Thanks, and good point! I have now removed that change from this PR.

@adamretter adamretter changed the title Improve build detect for RISCV and s390x Improve build detect for RISCV Feb 16, 2022
@adamretter
Copy link
Collaborator Author

@jay-zhuang @mrambacher can we get this one merged please?

@facebook-github-bot
Copy link
Contributor

@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@jay-zhuang jay-zhuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix.

@facebook-github-bot
Copy link
Contributor

@adamretter has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@XieJiSS
Copy link
Contributor

XieJiSS commented May 17, 2022

UPDATED 20230208: The issue raised in the following comment is mitigated by https://gitlab.archlinux.org/archlinux/devtools/-/merge_requests/119

FYI:

RISC-V has splitted its base I extension to (the new) I + Zicsr (for the csr register) + Zifencei (for fence.i), and now riscv64gc is riscv64imafdc_Zicsr_Zifencei (it used to be riscv64imafdc). This split might cause inconsistency. For instance, on a rv64gc SiFive Unmatched board, this bash command returns rv64imafdc. I believe this is a issue on the vendor's side, as those boards (specifically their dtbs) are unaware of the ISA change. To make things worse, there're a few patches for hwcaps etc. happening on Linux's side, which means that the format of /proc/cpuinfo may vary in near future. Further more, qemu-user provides the host machine's /proc/cpuinfo, causing the detect script to behave incorrectly, e.g. g++: error: ‘-march=fpu’: ISA string must begin with rv32 or rv64

However, I don't think these issues can be fixed soon. Currently, it is not possible to get the ISA of the machine for RISC-V using a distro-unaware way. For a specific distro like Arch Linux, one can use $CARCH or similar envs set by the build script, but AFAIK this env is not available for some other distros.

This inconsistency may not cause big problems, though, as the csr register is probably not useful for RocksDB, and the compiler might be able to work around the absent of fence.i instruction.

Handling -march for RISC-V can be tricky. Maybe we should left this script as-is until it breaks things. Downstream packagers can patch the detect script manually, like hard-coding their target arch's ISA string.

See also:

https://lore.kernel.org/linux-riscv/Ym0+Erz5DEnB78vu@Sun/
openssl/openssl#18197 (comment)

there is no way to detect zifencei/zicsr currently, while the toolchain has advanced so much that it mandates the usage of them

The kernel (e.g. Linux) still needs to offer some API to the userspace, one way is hwcaps, other ways are /proc/cpuinfo ad /proc/device-tree, which is not so acceptable as every userspace tool now has to bear a ISA string parser (seems no riscv-software-src/riscv-isa-parser though).

XieJiSS added a commit to felixonmars/archriscv-packages that referenced this pull request Aug 8, 2022
Shouldn't revert until qemu-user has its `/proc/cpuinfo` fixed.

Ref: facebook/rocksdb#9366
felixonmars pushed a commit to felixonmars/archriscv-packages that referenced this pull request Aug 8, 2022
Shouldn't revert until qemu-user has its `/proc/cpuinfo` fixed.

Ref: facebook/rocksdb#9366
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed RiscV64 Build or run RocksDB on Risc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants