libunwind: riscv: disable vector test when csr instructions aren't present

[xobs]

Summary

The commit ca0b4d58eaad405ea74b4db82ecb14b3cfdeccb7 (https://reviews.llvm.org/D136264) added a code path that uses the csrr instruction to read the vector length. This works fine when compiling under risc-v with the -g arch, such as rv32gc or rv64gc. However it will fail to compile when using a newer compiler under targets such rv32imac.

This patch adds a check for __riscv_zicsr as a compiler define.

https://reviews.llvm.org/D136264#4545515

[whitequark]

LGTM

[MaskRay]

This works fine when compiling under risc-v with the -g arch, such as rv32gc or rv64gc. However, depending on the vintage of the compiler, it does not work when compiling with an arch such as riscv32imac.

Regarding "depending on the vintage of the compiler" : My understanding is that only the latest one or two major releases of Clang, the latest Clang, and a very new GCC (currently 12) are officially supported.
So we shouldn't worry about the old versions. They are simply unsupported.

[xobs]

The other issue is that without this change, libunwind cannot compile on targets such as rv32imac since that instruction set does not include csrr as an opcode.

If you like (and if it's possible to do so), I can update the description to reflect that fact. Or we can gate it on __riscv_zicsr instead, as suggested in https://reviews.llvm.org/D136264#4545515

[MaskRay]

In D156642#4545571, @xobs wrote:

The other issue is that without this change, libunwind cannot compile on targets such as rv32imac since that instruction set does not include csrr as an opcode.

If you like (and if it's possible to do so), I can update the description to reflect that fact. Or we can gate it on __riscv_zicsr instead, as suggested in https://reviews.llvm.org/D136264#4545515

The question is related to what configurations libunwind needs to support. I think this is unfortunately a bit vague.
Can you describe your configuration by providing detailed cmake command lines?

[xobs]

My configuration is building libunwind via the Rust build system. This links libunwind into compiled programs in order to unwind the stack.

The build command is:

running: "riscv-none-elf-gcc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-march=rv32imac" "-mabi=ilp32" "-mcmodel=medany" "-static" "-I" "/opt/Xous/rust-next/src/llvm-project/libunwind/include" "-nostdinc++" "-fno-exceptions" "-fno-rtti" "-fstrict-aliasing" "-funwind-tables" "-fvisibility=hidden" "-fno-stack-protector" "-ffreestanding" "-U_FORTIFY_SOURCE" "-std=c++11" "-D_LIBUNWIND_DISABLE_VISIBILITY_ANNOTATIONS" "-D_FORTIFY_SOURCE=0" "-DRUST_SGX=1" "-D__NO_STRING_INLINES" "-D__NO_MATH_INLINES" "-D_LIBUNWIND_IS_BAREMETAL" "-D__LIBUNWIND_IS_NATIVE_ONLY" "-D_LIBUNWIND_REMEMBER_HEAP_ALLOC" "-DNDEBUG" "-o" "/opt/Xous/rust-next/build/riscv32imac-unknown-xous-elf/native/libunwind/libunwind.o" "-c" "/opt/Xous/rust-next/src/llvm-project/libunwind/src/libunwind.cpp"
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp: Assembler messages:
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr a0,0xC22'
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr a5,0xC22'
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr a4,0xC22'
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr a0,0xC22'
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr s4,0xC22'
cargo:warning=/opt/Xous/rust-next/src/llvm-project/libunwind/src/Registers.hpp:4106: Error: unrecognized opcode `csrr a0,0xC22'
exit status: 1

The arch is either "rv32imac" or "rv32imac_zicsr" depending on the underlying version of the compiler, however note that "rv32imac" is still a perfectly valid target -- it just has no CSR opcodes.

Ultimately I would like this to build for "rv32imac", and the patch provided accomplishes that.

[asb]

Thanks for the patch @xobs - you're right there's a problem here, but I think your current approach needs a slight tweak. __riscv_v corresponds to a specific set of the vector extension - you'd be better off checking for __riscv_zicsr as you suggest earlier in the thread, or __riscv_vector, which is defined if V or any of the Zve* extensions is available (see here).

On a sidenote: all the other registers are copied into Registers_riscv and then read from there - is it correct that vlenb is read directly through a CSR read on demand?

I think one reason for confusion earlier in the thread is that Clang/LLVM and GCC handled the change that moved Zicsr out of the base instruction set differently. As documented here, the CSR instructions are unconditionally enabled in LLVM currently. So this patch is addressing a GCC incompatibility.

[xobs]

Change define gate from __riscv_v to __riscv_zicsr

[xobs]

Thanks for the input! I didn't realise that clang was making an explicit choice to keep those instructions in the base ISA, which certainly explains why I haven't seen any issues in Rust, and why they haven't renamed all of the targets to include`_zicsr`.

Given all that, I think that the decision to check for the __riscv_zicsr gate is the correct choice, and I've updated the patch accordingly.

[kito-cheng]

libunwind/src/Registers.hpp

4107

Another alternative is using .insn for this to prevent any dependency of extension, this should be generally OK since when program hit there means vector extension is enabled, which means csrr is available, and AArch64 has use this trick in their libgcc implementation[1], although I guess their intention is compatibility issue with older binutils , but I think we could use same trick.

This might be more robust implantation since we also treat UNW_RISCV_VLENB as valid register in other place like line 4088.

So I think either:

1. Check with __riscv_zicsr for UNW_RISCV_VLENB, but also check all other use site for UNW_RISCV_VLENB.
2. Use .insn.

[1] https://github.com/gcc-mirror/gcc/blob/master/libgcc/config/aarch64/value-unwind.h#L38

libunwind/src/Registers.hpp

inline reg_t Registers_riscv::getRegister(int regNum) const {
   if (regNum == UNW_REG_IP)
     return _registers[0];
   if (regNum == UNW_REG_SP)
     return _registers[2];
   if (regNum == UNW_RISCV_X0)
     return 0;
   if ((regNum > 0) && (regNum < 32))
     return _registers[regNum];
+#if defined(__riscv_zicsr)
   if (regNum == UNW_RISCV_VLENB) {
     reg_t vlenb;
     __asm__("csrr %0, 0xC22" : "=r"(vlenb));
     return vlenb;
   }
+#endif
   _LIBUNWIND_ABORT("unsupported riscv register");
 }
 
 inline void Registers_riscv::setRegister(int regNum, reg_t value) {
   if (regNum == UNW_REG_IP)
     _registers[0] = value;
   else if (regNum == UNW_REG_SP)
     _registers[2] = value;