Re: [PATCH] target/avr: Use vaddr type for $PC jumps

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 9/10/25 23:42, Richard Henderson wrote:
> On 10/9/25 13:05, Philippe Mathieu-Daudé wrote:
>> translator_use_goto_tb() expects a vaddr type since commit
>> b1c09220b4c ("accel/tcg: Replace target_ulong with vaddr in
>> translator*()").
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   target/avr/translate.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/target/avr/translate.c b/target/avr/translate.c
>> index 804b0b21dbd..20191055861 100644
>> --- a/target/avr/translate.c
>> +++ b/target/avr/translate.c
>> @@ -87,7 +87,7 @@ struct DisasContext {
>>       CPUAVRState *env;
>>       CPUState *cs;
>> -    target_long npc;
>> +    vaddr npc;
> 
> Ah, here's where proper typing might have saved us a bug.
> 
> npc is not a virtual (or physical) address in the normal sense, it is a 
> *word* address (i.e. byte address / 2).
> 
> So I think you should just use uint32_t here.

IIUC the field is 24-bit wide, OK.

> 
>>       uint32_t opcode;
>>       /* Routine used to access memory */
>> @@ -981,7 +981,7 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
>>       }
>>   }
>> -static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
>> +static void gen_goto_tb(DisasContext *ctx, int n, vaddr dest)
>>   {
>>       const TranslationBlock *tb = ctx->base.tb;
>> @@ -1004,7 +1004,7 @@ static void gen_goto_tb(DisasContext *ctx, int 
>> n, target_ulong dest)
>>    */
>>   static bool trans_RJMP(DisasContext *ctx, arg_RJMP *a)
>>   {
>> -    int dst = ctx->npc + a->imm;
>> +    vaddr dst = ctx->npc + a->imm;
> 
> And here...
> 
>>       gen_goto_tb(ctx, 0, dst);
> 
> ... and therefore also in the gen_goto_tb argument.
> 
> The bug can thus be said to be within gen_goto_tb, where we don't 
> convert from word address to byte address.

Oh I see. Then avr_tr_insn_start() is also buggy?