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

Bruno Larsen via Gdb-patches
2023-07-21T07:25:05Z

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