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;
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);
--
2.31.1
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~
© 2016 - 2024 Red Hat, Inc.