search

riscv-collab / riscv-gnu-toolchain

GNU toolchain for RISC-V, including GCC
Other
3.54k stars 1.16k forks source link

Is `setjmp.h` reserving space for FP registers unnecessarily? #1555

Closed TommyMurphyTM1234 closed 1 month ago

TommyMurphyTM1234 commented 1 month ago

Apropos of an issue that arose here on the ch32v003fun repo:

At the moment riscv64-unknown-elf/include/machine/setjmp.h (I presume that the Linux toolchain file is similar or the same) has this:

#ifdef __riscv
/* _JBTYPE using long long to make sure the alignment is align to 8 byte,
   otherwise in rv32imafd, store/restore FPR may mis-align.  */
#define _JBTYPE long long
#ifdef __riscv_32e
#define _JBLEN ((4*sizeof(long))/sizeof(long))
#else
#define _JBLEN ((14*sizeof(long) + 12*sizeof(double))/sizeof(long))
#endif
#endif

However this seems to mean that if a non floating point enabled architecture other than rv32e is in use at preprocessing time then space will be (unnecessarily?) reserved for floating point registers.

Should the architecture checks be extended so that this does not happen?

aswaterman commented 1 month ago

It is over-sized for the ilp32/ilp32f/lp64/lp64f calling conventions. (Note the ISA doesn't matter, just the calling convention.)

Unfortunately, the size of jmp_buf is exposed through the ABI, so adjusting it is an ABI break. Even though that's less of a concern for an embedded toolchain, it's probably best to leave this alone.

TommyMurphyTM1234 commented 1 month ago

Thanks @aswaterman.

Would anybody have a link to any prior material/discussions explaining how this implementation (and in particular, the 4, 14 and 12 values) was arrived at?

Also, is the comment about _JBTYPE correct? If it's the ABI and not the architecture that matters here why does it mention an architecture, and why rv32imafd specifically?

aswaterman commented 1 month ago

I can explain the constants. For RV32E, 4 = pc + sp + s0 + s1. For RV32I/RV64I, 14 = pc + sp + s0..s11, and 12 = fs0..fs11.

See the struct defn in glibc for additional clarity: https://github.com/bminor/glibc/blob/6f3f6c506cdaf981a4374f1f12863b98ac7fea1a/sysdeps/riscv/bits/setjmp.h#L22

The comment you mentioned probably predates separating the calling conventions from the ISA. ilp32d would probably be more appropriate.

TommyMurphyTM1234 commented 1 month ago

Thanks a lot @aswaterman - that's very helpful. 👍

TommyMurphyTM1234 commented 1 month ago

Closing this since even if it might be desirable, it doesn't seem feasible to modify the upstream GCC setjmp.h definition of _JBTYPE. In relation to the original ch32v003fun issue that prompted this I have suggested that they use a local/custom fix if necessary: