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

Pedro Alves
2023-01-24T19:21:50Z

On 2023-01-24 6:25 p.m., Carl Love wrote:

> I have not tested with clang. Actually I have never used clang so this
> is another thing to look at and test.
>
>
> Let me see if I can summarize the current situation.
>
> 1) The goal of the current X86 reverse patch is to fix the case where
> we have called function foo () and are trying to do a reverse-finish
> from inside the function.

Wrong goal. There's nothing to fix there. The command is working as designed.

> The mainline gdb code stops at the entry to
> foo () then does a single instruction in the reverse direction to get
> to the caller. Specifically, the code that this patch removes:
>
> - if (ecs->event_thread->control.proceed_to_finish
> - && execution_direction == EXEC_REVERSE)
> - {
> - struct thread_info *tp = ecs->event_thread;
> -
> - /* We are finishing a function in reverse, and just hit the
> - step-resume breakpoint at the start address of the
> - function, and we're almost there -- just need to back up
> - by one more single-step, which should take us back to the
> - function call. */
> - tp->control.step_range_start = tp->control.step_range_end = 1;
> - keep_going (ecs);
> - return;
>
> The single-step in gdb results in stopping at the last instruction in
> the line where function foo is called. The result here is that you now
> need to do two reverse-next instructions to get to the previous line.

And that is the bug. reverse-next/reverse-step should _never_ stop at the same line.
Same as forward step/next. For instance, this "next" command will just continue
stepping forever:

Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc98) at reverse.c:11
11 while (1);
(gdb) next

Same for "step" instead of "next".

It turns out that reverse-next/step do stop immediately!

(gdb) reverse-next
11 while (1);
(gdb) reverse-next
11 while (1);
(gdb)

That's probably caused by the same bug.

> The command sequence is: reverse-finish; reverse-next; reverst-next to
> go from inside the function to the previous line in the caller.
>
> Note, in this case you are in the function and trying to return to the
> caller with the reverse-finish command. That is a different scenario
> from what Pedro is talking about in 2) below.
>
> 2) The scenario that Pedro brings up is a reverse-next over a line with
> two function calls on the same source line, i.e.
>
>> 9 int main ()
>> 10 {
>> 11 func1 (); func2 ();
>> 12 }
>
> In this case you are in the caller and are trying to do a reverse-next
> over the two function callers. This is a different scenario from 1).
>

I know it's a different scenario, but my scenario is just a way to show
up that the root of the problem can be seen with a simpler scenario. The
problem is in the reverse-step command, and you are focusing on the reverse-finish
command, incorrectly. Please read my responses to Bruno as well.

> 3) The bug that I mentioned earlier for the case of
> multiple executable statements on the same line.
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28426
>
> is for a case like:
>
> int main ()
> {
> int a, b, c, d;
> a = 1;
> b = 2;
> c = 3; d = 4;
> a = a + b + c + d;
> }
>
> In this case the reverse-next from the last line a = a + b + c + d; is
> not stepping all the way back over the line c = 3; d = 4;. This is a
> simpler version of 2). Specifically the command sequence to step over
> line c = 3; d = 4; is reverse-next, reverse-next. Only one reverse-
> next should be required.
>

Exactly. It's all the same bug.

> The patch that Luis and I have worked on fixes this issue, however it
> does not fix the case of the multiple statements being function calls,
> i.e. 2) above that Pedro is bringing up.

I have said it numerous times now, but it's worth repeating. It's all
the same bug. :-) Fix reverse-step, and then the reverse-finish
scenario fixes itself, because you will no longer need to issue
two reverse-step commands after reverse-finish.

>
>
> From a subsequent message from Pedro in this thread:
>
> On Tue, 2023-01-24 at 15:06 +0000, Pedro Alves wrote:
> > Yes. And the reason you need two "reverse-step" is because there are
> > two line ranges for line 8, and reverse-step stops
> > at the beginning of the second range instead of at the beginning of
> > the first. It's the exact same as with my simpler
> > example of just doing "next" -> "reverse-next", which I used as a
> > simpler example to explain the problem.
> >
> > > What Carl is proposing we do is make it so GDB only needs one
> > > command.
> >
> > I understand. However, I am saying that that is papering over the
> > actual problem, _and_ it only works in the situation
> > where you ended up with two line entries with is-stmt for the same
> > line. Note how the patch breaks clang, and gcc with
> > -gno-column-info...
>
> I don't agree that I am "papering over the actual problem" rather I
> think at this point that Pedro's test case is an additional case of the
> reverse-next command being broken by my patch to fix the reverse-finish
> command. The problem doesn't exist without my patch.
>
>
> So at this point, I need to go see if I can figure out how the patch to
> fix the reverse-finish command causes the regression in 2) for the
> reverse-next command.

At this point. NAK on the reverse-finish fix.

>
> Looks like we also need an to add an additional test case for 2).
> Also, will need to look at how any new fix for 2) behaves with clang.
>
> Thanks for all the input on this issue. It seems that there is still
> work to do.