| Message ID | 20210926213902.1713506-1-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | tcg/riscv: Fix potential bug in clobbered call register set | expand |
On 9/26/21 5:39 PM, Philippe Mathieu-Daudé wrote: > The tcg_target_call_clobber_regs variable is of type TCGRegSet, > which is unsigned and might be 64-bit wide. By initializing it > as unsigned type, only 32-bit are set. Currently the RISCV TCG > backend only uses 32 registers, so this is not a problem. > However if more register were to be implemented (such vectors) > then it would become problematic. Since we are better safe than > sorry, properly initialize the value as 64-bit. > > Fixes: 7a5549f2aea ("tcg/riscv: Add the target init code") > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > tcg/riscv/tcg-target.c.inc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc > index dc8d8f1de23..5bd95633b0d 100644 > --- a/tcg/riscv/tcg-target.c.inc > +++ b/tcg/riscv/tcg-target.c.inc > @@ -1734,7 +1734,7 @@ static void tcg_target_init(TCGContext *s) > tcg_target_available_regs[TCG_TYPE_I64] = 0xffffffff; > } > > - tcg_target_call_clobber_regs = -1u; > + tcg_target_call_clobber_regs = -1ull; There are not 64 registers, so this is incorrect. I don't think your logic is correct. r~
On 9/27/21 01:06, Richard Henderson wrote: > On 9/26/21 5:39 PM, Philippe Mathieu-Daudé wrote: >> The tcg_target_call_clobber_regs variable is of type TCGRegSet, >> which is unsigned and might be 64-bit wide. By initializing it >> as unsigned type, only 32-bit are set. Currently the RISCV TCG >> backend only uses 32 registers, so this is not a problem. >> However if more register were to be implemented (such vectors) >> then it would become problematic. Since we are better safe than >> sorry, properly initialize the value as 64-bit. >> >> Fixes: 7a5549f2aea ("tcg/riscv: Add the target init code") >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> tcg/riscv/tcg-target.c.inc | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc >> index dc8d8f1de23..5bd95633b0d 100644 >> --- a/tcg/riscv/tcg-target.c.inc >> +++ b/tcg/riscv/tcg-target.c.inc >> @@ -1734,7 +1734,7 @@ static void tcg_target_init(TCGContext *s) >> tcg_target_available_regs[TCG_TYPE_I64] = 0xffffffff; >> } >> - tcg_target_call_clobber_regs = -1u; >> + tcg_target_call_clobber_regs = -1ull; > > There are not 64 registers, so this is incorrect. Currently there are 32 registers, but I was looking at this draft: https://five-embeddev.com/riscv-v-spec/draft/v-spec.html#_vector_registers "The vector extension adds 32 architectural vector registers, v0-v31 to the base scalar RISC-V ISA." If this were to be implemented (and available on the host), wouldn't we have 64 registers? > I don't think your logic is correct. Eventually this line would be easier to review as: tcg_target_call_clobber_regs = MAKE_64BIT_MASK(0, TCG_TARGET_NB_REGS);
On 9/27/21 1:36 AM, Philippe Mathieu-Daudé wrote: >> There are not 64 registers, so this is incorrect. > > Currently there are 32 registers, but I was looking at this draft: > https://five-embeddev.com/riscv-v-spec/draft/v-spec.html#_vector_registers > "The vector extension adds 32 architectural vector registers, v0-v31 > to the base scalar RISC-V ISA." > If this were to be implemented (and available on the host), wouldn't > we have 64 registers? Sure. But there are *lots* of changes required before that happens, and certainly you shouldn't be assuming what the ABI is now. > Eventually this line would be easier to review as: > > tcg_target_call_clobber_regs = MAKE_64BIT_MASK(0, TCG_TARGET_NB_REGS); Would it? Or would it be eaier to review with tcg_target_call_clobber_regs = 0; followed by a set of each register that is call clobbered. Why are you assuming that it's safer to list unknown registers as call-clobbered? IF ANYTHING, it might be safer to assume that all new registers are caller saved. But as a general principal, I also don't like register masks containing set bits outside the range of the mask. r~
diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc index dc8d8f1de23..5bd95633b0d 100644 --- a/tcg/riscv/tcg-target.c.inc +++ b/tcg/riscv/tcg-target.c.inc @@ -1734,7 +1734,7 @@ static void tcg_target_init(TCGContext *s) tcg_target_available_regs[TCG_TYPE_I64] = 0xffffffff; } - tcg_target_call_clobber_regs = -1u; + tcg_target_call_clobber_regs = -1ull; tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_S0); tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_S1); tcg_regset_reset_reg(tcg_target_call_clobber_regs, TCG_REG_S2);
The tcg_target_call_clobber_regs variable is of type TCGRegSet, which is unsigned and might be 64-bit wide. By initializing it as unsigned type, only 32-bit are set. Currently the RISCV TCG backend only uses 32 registers, so this is not a problem. However if more register were to be implemented (such vectors) then it would become problematic. Since we are better safe than sorry, properly initialize the value as 64-bit. Fixes: 7a5549f2aea ("tcg/riscv: Add the target init code") Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- tcg/riscv/tcg-target.c.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)