[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

[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

[Steven Seeger]

>From ef3183a1648f45c705b55704de3755f84e9bcf80 Mon Sep 17 00:00:00 2001
From: Steven Seeger <steven.seeger@flightsystems.net>
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 <steven.seeger@flightsystems.net>
---
 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);
         }
-- 
2.13.6

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
<steven.seeger@flightsystems.net> wrote:
> From ef3183a1648f45c705b55704de3755f84e9bcf80 Mon Sep 17 00:00:00 2001
> From: Steven Seeger <steven.seeger@flightsystems.net>
> 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 <steven.seeger@flightsystems.net>
> ---
>  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

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

[Steven Seeger]

On Thursday, February 15, 2018 2:39:34 PM EST Peter Maydell wrote:
> On 15 February 2018 at 19:15, Steven Seeger
> 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

Hi Peter. Thank you for the thoughtful reply. I did not consider the 
possibility that in this case the instruction should execute twice. The issue 
here is that when stepping through code in gdb, after the branch to the delay 
slot, pc==npc. So subsequent steps actually go nowhere. This could maybe be a 
problem with GDB, but I don't think so.

I will have access to real hardware Tuesday and will test this case.

If the instruction should execute twice, then we may need to kick npc in qemu 
in order to alleviate this.

Steven

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

[Steven Seeger]

Oh, let me also add that this assertion fails in gdb: 

Thread 1 hit Breakpoint 3, 0x40f102d8 in ?? ()
(gdb) si
./../../src/gdb-8.1/gdb/sparc-tdep.c:1737: internal-error: std::vector<long 
unsigned int> sparc_software_single_step(regcache*): Assertion
`nnpc != npc || orig_npc == 0' failed.

I had this assertion commented out so when I unrolled my patch to repeat the 
test in order to write my previous reply to you, I saw it be stuck on the same 
instruction rather than fail this assertion.

I guess this could be a gdb bug, too.

Steven

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

[Steven Seeger]

All,

I think that the issue I pointed out the list is not a QEMU bug at all but a 
GDB bug. GDB's sparc software single step implementation seems to be what is 
stuck. It always re-sets the breakpoint at the current instruction in this 
case. It calculates the breakpoint offsets by analyzing the instruction and 
doesn't take this edge case into account. It does not appear to receive an 
invalid npc value from QEMU as I thought it was.

Steven