RFR: 8346706: RISC-V: Add available registers to hs_err [v2]

[Robbin Ehn]

On Mon, 30 Dec 2024 04:55:38 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Review comments
>>  - Make file fixes
>
> src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp line 369:
> 
>> 367: // No other choice but to define it.
>> 368: #ifndef RISCV_V_MAGIC
>> 369: #define RISCV_V_MAGIC 0x53465457
> 
> It might help to have some extra code comment about where it is defined.

Added it, but the issue is that in 10y it may have been moved in the kernel.

> src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp line 387:
> 
>> 385:   uint32_t ext_size = ext->hdr.size;
>> 386: 
>> 387:   if (ext_size < (sizeof(struct __riscv_ctx_hdr) + sizeof(*v_ext_state))) {
> 
> I am not sure, but can we check for equality about the size here? Personally, I prefer `sizeof(struct __riscv_v_ext_state)` to `sizeof(*v_ext_state)`. I mean something like this:
> 
> if (ext_size == (sizeof(struct __riscv_ctx_hdr) + sizeof(struct __riscv_v_ext_state))) {

We can't check for equality as we don't know how much space the vector registers uses here.
In general sizeof pointer points to is less error prone as if you change the pointer type the sizeof is still correct.
But in this case I do agree with you as there are so many 'special' structs it's hard to keep track anyways.

> src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp line 401:
> 
>> 399:   st->print_cr("vtype=" INTPTR_FORMAT, v_ext_state->vtype);
>> 400:   st->print_cr("vcsr=" INTPTR_FORMAT, v_ext_state->vcsr);
>> 401:   st->print_cr("vlenb=" INTPTR_FORMAT, v_ext_state->vlenb);
> 
> Maybe add some formatting here so that the numbers printed are on the same colume.

Sure

> src/hotspot/os_cpu/linux_riscv/os_linux_riscv.cpp line 406:
> 
>> 404:   uint64_t vr_size = v_ext_state->vlenb;
>> 405: 
>> 406:   if (ext_size < (32 * vr_size)) {
> 
> Similar here. Can we check for equality about the size here?
> 
> if (ext_size == (32 * vr_size)) {

Yes, here we can check.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22845#discussion_r1905414568
PR Review Comment: https://git.openjdk.org/jdk/pull/22845#discussion_r1905413338
PR Review Comment: https://git.openjdk.org/jdk/pull/22845#discussion_r1905414001
PR Review Comment: https://git.openjdk.org/jdk/pull/22845#discussion_r1905413845