Re: [PATCH v2] target/arm: Fix assert on BRA.

[Richard Henderson <richard.henderson@linaro.org>]

On 11/28/25 01:31, Peter Maydell wrote:
> On Fri, 28 Nov 2025 at 04:38, Harald van Dijk <hdijk@accesssoftek.com> wrote:
>>
>> trans_BRA does
>>
>>      gen_a64_set_pc(s, dst);
>>      set_btype_for_br(s, a->rn);
>>
>> gen_a64_set_pc does
>>
>>      s->pc_save = -1;
>>
>> set_btype_for_br (if aa64_bti is enabled and the register is not x16 or
>> x17) does
>>
>>      gen_pc_plus_diff(s, pc, 0);
>>
>> gen_pc_plus_diff does
>>
>>      assert(s->pc_save != -1);
>>
>> Hence, this assert is getting hit. We need to call set_btype_for_br
>> before gen_a64_set_pc, and there is nothing in set_btype_for_br that
>> depends on gen_a64_set_pc having already been called, so this commit
>> simply swaps the calls.
>>
>> Signed-off-by: Harald van Dijk <hdijk@accesssoftek.com>
>> ---
>>   target/arm/tcg/translate-a64.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/tcg/translate-a64.c b/target/arm/tcg/translate-a64.c
>> index 08b21d7dbf..cde22a5cca 100644
>> --- a/target/arm/tcg/translate-a64.c
>> +++ b/target/arm/tcg/translate-a64.c
>> @@ -1916,8 +1916,8 @@ static bool trans_BRA(DisasContext *s, arg_bra *a)
>>           return false;
>>       }
>>       dst = auth_branch_target(s, cpu_reg(s,a->rn), cpu_reg_sp(s, a->rm), !a->m);
>> -    gen_a64_set_pc(s, dst);
>>       set_btype_for_br(s, a->rn);
>> +    gen_a64_set_pc(s, dst);
>>       s->base.is_jmp = DISAS_JUMP;
>>       return true;
>>   }
> 
> The commit message on commit 64678fc45d8f6 says
>      The set_btype_for_br call must be moved after the gen_a64_set_pc
>      call to ensure the current pc can still be computed.
> 
> but I think that is incorrect and it meant to say "moved before",
> because the actual code changes it makes to trans_BR() and
> trans_BRAZ() are
> 
> @@ -1521,8 +1528,8 @@ static void set_btype_for_blr(DisasContext *s)
> 
>   static bool trans_BR(DisasContext *s, arg_r *a)
>   {
> -    gen_a64_set_pc(s, cpu_reg(s, a->rn));
>       set_btype_for_br(s, a->rn);
> +    gen_a64_set_pc(s, cpu_reg(s, a->rn));
>       s->base.is_jmp = DISAS_JUMP;
>       return true;
>   }
> @@ -1581,8 +1588,8 @@ static bool trans_BRAZ(DisasContext *s, arg_braz *a)
>       }
> 
>       dst = auth_branch_target(s, cpu_reg(s, a->rn), tcg_constant_i64(0), !a->m);
> -    gen_a64_set_pc(s, dst);
>       set_btype_for_br(s, a->rn);
> +    gen_a64_set_pc(s, dst);
>       s->base.is_jmp = DISAS_JUMP;
>       return true;
>   }
> 
> which move the set_btype_for_br() call to before gen_a64_set_pc().
> 
> So I think that we just forgot to also include trans_BRA() in
> that change, and your patch here fixes that.
> 
> Richard,  does that sound right?

Yep.

> 
> If so, this should be:
> Cc: qemu-stable@nongnu.org
> Fixes: 64678fc45d8f6 ("target/arm: Fix BTI versus CF_PCREL")
> 
> and you can have
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~