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

[Carl Love <cel@us.ibm.com>]

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