Re: [Qemu-devel] [PATCH 01/02] fix issue where a branch to pc+4 confuses GDB because pc and npc are set to the same value

[Peter Maydell]

On 15 February 2018 at 19:15, Steven Seeger
<address@hidden> wrote:
> From ef3183a1648f45c705b55704de3755f84e9bcf80 Mon Sep 17 00:00:00 2001
> From: Steven Seeger <address@hidden>
> Date: Thu, 15 Feb 2018 13:20:04 -0500
> Subject: [PATCH 1/2] fix issue where a branch to pc+4 confuses GDB because pc
>  and npc are set to the same value
>
> Signed-off-by: Steven Seeger <address@hidden>
> ---
>  target/sparc/translate.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 71e0853e43..4c2272003d 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -1464,6 +1464,9 @@ static void do_branch(DisasContext *dc, int32_t offset,
> uint32_t insn, int cc)
>              dc->npc = dc->pc + 4;
>          } else {
>              dc->pc = dc->npc;
> +            if (target == dc->pc) {
> +                target += 4;
> +            }
>              dc->npc = target;
>              tcg_gen_mov_tl(cpu_pc, cpu_npc);
>          }
> @@ -1504,6 +1507,9 @@ static void do_fbranch(DisasContext *dc, int32_t offset,
> uint32_t insn, int cc)
>              dc->npc = dc->pc + 4;
>          } else {
>              dc->pc = dc->npc;
> +            if (target == dc->pc) {
> +                target += 4;
> +            }
>              dc->npc = target;
>              tcg_gen_mov_tl(cpu_pc, cpu_npc);

These changes look rather odd -- are you sure they're right?
This is the code for unconditional taken branch, not annulled, and
my copy of the sparc architecture manual says that in that case
the new PC value should be the old nPC value, and the new
nPC value should be the effective address of the branch target.
There's nothing in there about branches into your own delay
slot being a special case. Adding 4 to target like this will
make the new nPC value be 4 further forward, which would mean
we'd only execute the branch target instruction once, rather
than twice (once for it being in the branch delay slot and
once as the instruction target).

It's a weird thing to do so I wouldn't be surprised if gdb
mishandled it. Have you tested against real sparc hardware?

thanks
-- PMM