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

[Bruno Larsen <blarsen@redhat.com>]

On 20/07/2023 16:45, Carl Love wrote:
> 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???

I just tested with the newest version of the patch relating to reverse 
stepping over contiguous lines, but that didn't make a difference. GCC 
continued broken. I think the problem is that GCC doesn't have 
contiguous ranges, it has a single range that doesn't contain the PC

My (probably naive) thinking is to set step_range_end to the starting PC 
of the following line, and step_range_start to be the real start of the 
current line at the moment the command is parsed. This sounds like it 
would solve the GCC problem, but I assume I'm missing something. 
Unfortunately, I don't have the time to test this theory myself, but 
I'll happily test any patches you submit :). If you want to repeat my 
test, I want the behavior of gdb.reverse/finish-reverse-next from gcc to 
match clang's (on x86 machines), that is, if you reverse finish from 
function1, a single "rn" command would land you on the b=5 line.

-- 
Cheers,
Bruno