RE: [PATCH 2/2 ] PowerPC: fix for gdb.reverse/finish-precsave.exp and gdb.reverse/finish-reverse.exp

Carl Love via Gdb-patches
2023-07-20T14:46:00Z

Bruno:

On Thu, 2023-07-20 at 14:01 +0200, Bruno Larsen wrote:
> (gdb) disas /s
> (snip)
> 81 b = 5;
> 0x00000000004011bb <+43>: movl $0x5,-0x18(%rbp)
>
> 82
> 83 function1 (a, b); // CALL VIA LEP
> 0x00000000004011c2 <+50>: mov -0x14(%rbp),%edi
> 0x00000000004011c5 <+53>: mov -0x18(%rbp),%esi
> => 0x00000000004011c8 <+56>: call 0x401140 <function1>
> (gdb) rn
> 81 b = 5;
>
> Basically, the best way to fix this solution is to get reverse-next
> and
> reverse-step to properly figure out the stepping range and not have
> a
> reverse-next that ends up in the same line

---------------------

Per my reply to Simon in "Re: [EXTERNAL] Re: [PATCH 2/2 v5] Fix reverse
stepping multiple contiguous PC ranges over the line table." on
6/23/2023. Where he was wondering why control.step_range_start was not
set to the "real" range....

This is the code in the Patch:
>
> > +{
> > + /* The line table may have multiple entries for the same source
> > code line.
> > + Given the PC, check the line table and return the PC that
> > corresponds
> > + to the line table entry for the source line that PC is
> > in. */
> > + CORE_ADDR start_line_pc = ecs->event_thread-
> > >control.step_range_start;
> > + gdb::optional<CORE_ADDR> real_range_start;
> > +
> > + /* Call find_line_range_start to get the smallest address in the
> > + linetable for multiple Line X entries in the line table. */
> > + real_range_start = find_line_range_start (pc);
> > +
> > + if (real_range_start.has_value ())
> > + start_line_pc = *real_range_start;
> > +
> > + return start_line_pc;

Simon's comment about the code:

>
> When I read this, I wonder: why was control.step_range_start not set
> to
> the "real" range start in the first place (not only in the context of
> reverse execution, every time it is set)? It would seem more robust
> than patching it afterwards in some very specific spots.
>
> I could see some benefits for range-stepping uses cases too (relevant
> when debugging remotely). Using your example here:
>
> Line X - [0x0 - 0x8]
> Line X - [0x8 - 0x10]
> Line X - [0x10 - 0x18]
>
> Imagine we are stopped at 0x14, and we type "next", and 0x14 is a
> conditional jump to 0x5. It seems like current GDB would send a
> "range
> step" request to GDBserver, to step in the [0x10, 0x18[ range. When
> reaching 0x5, execution would stop, and GDB would resume it again
> with
> the [0x0,0x8[ range. When reaching 0x8, it would stop again, GDB
> would
> resume it with [0x8,0x10[, and so on. If GDB could send a "range
> step"
> request with the [0x0,0x18[ range, it would avoid those unnecessary
> intermediary stop.
>

My reply to Simon's comment:

We looked at trying to set control.step_range_start to the real start
range initially. Ulrich brought this point up in our internal review
of the patch.

So, when I am in function finish_backward() in infcmd.c I have no way
to determine what the previous PC was. If I assume it was the previous
value, i.e. pc - 4byes (on PowerPC). I get a gdb internal error. It
seems that I am not allowed to change the line range to something that
does not include the current pc value.

../../binutils-gdb-reverse-multiple-contiguous/gdb/infrun.c:2740:
internal-error: resume_1:
Assertion `pc_in_thread_step_range (pc, tp)' failed.

In order to make that work, we concluded that it would probably entail
a much bigger change to how reverse execution works which would be
beyond the scope of what this patch is trying to fix. So, being able
to do what I believe you want to do is in theory possible but it would
require a larger, independent change to what this patch is trying to
fix.

---------------------------------------

I think what Bruno is again asking is to have control.step_range_start
set to the real start range initially, i.e. what Simon asked about. I
think to do that, we would need to make significant changes to how
reverse execution works to allow us to make that change. It didn't
appear to be a straight forward fix to me. I may be wrong. Maybe
someone sees a good way to make that work that I am missing. So it
looks like this patch fixes most issues but not all of the problems
with reverse-step and reverse-next. It might be good to try and fix
this additional scenario in a separate patch, not sure???

Carl