[PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Philippe Mathieu-Daudé]

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

[PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Philippe Mathieu-Daudé]

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

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Richard Henderson]

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~

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Richard Henderson]

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~

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Philippe Mathieu-Daudé]

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);

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Philippe Mathieu-Daudé]

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);

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Richard Henderson]

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~

Re: [PATCH] tcg/riscv: Fix potential bug in clobbered call register set

[Richard Henderson]

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~