| Message ID | 5193436.NpsRQSl3SI@wirbelwind |
|---|---|
| State | New |
| Headers | show |
| Series | [01/02] fix issue where a branch to pc+4 confuses GDB because pc and npc are set to the same value | expand |
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
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
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
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
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);