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

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

[Harald van Dijk]

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;
 }
-- 
2.47.3

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

[Peter Maydell]

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?

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>

thanks
-- PMM

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

[Richard Henderson]

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~

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

[Peter Maydell]

On Sun, 30 Nov 2025 at 20:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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>


Applied to target-arm.next, thanks.

-- PMM