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

Carl Love via Gdb-patches
2023-03-13T17:32:34Z

Ulrich:

On Mon, 2023-03-13 at 14:16 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
>
> > This patch fixes the reverse-finish command on PowerPC. The
> > command
> > now works the same as on other architectures, specifically
> > X86. There
> > are no functional changes for other architectures. The patch
> > includes
> > a new testcase to verify the reverse-finish command works correctly
> > with the multiple entry points supported by PowerPC.
>
> This looks good to me in general, just one cosmetic issue:
>
> > - /* Set a step-resume at the function's entry point. Once
> > that's
> > - hit, we'll do one more step backwards. */
> > + if ((pc < alt_entry_point) || (pc > entry_point))
> > + {
> > + /* We are in the body of the function. Set a breakpoint to
> > go back to
> > + the normal entry point. */
> > symtab_and_line sr_sal;
> > - sr_sal.pc = sal.pc;
> > + sr_sal.pc = entry_point;
> > sr_sal.pspace = get_frame_program_space (frame);
> > - insert_step_resume_breakpoint_at_sal (gdbarch,
> > - sr_sal, null_frame_id);
> > -
> > - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> > + insert_step_resume_breakpoint_at_sal (gdbarch, sr_sal,
> > + null_frame_id);
> > }
> > +
> > else
> > - {
> > - /* We're almost there -- we just need to back up by one more
> > - single-step. */
> > - tp->control.step_range_start = tp->control.step_range_end =
> > 1;
> > - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> > - }
> > + /* We are either at one of the entry points or between the
> > entry points.
> > + If we are not at the alt_entry point, go back to the
> > alt_entry_point
> > + If we at the normal entry point step back one instruction,
> > when we
> > + stop we will determine if we entered via the entry point or
> > the
> > + alternate entry point. If we are at the alternate entry
> > point,
> > + single step back to the function call. */
> > + tp->control.step_range_start = tp->control.step_range_end = 1;
> > +
> > + proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
>
> There's still a bunch of unnecessary changes in there (changes
> in formatting, moving the "proceed" call, etc.). It would be
> preferable to not have those in.

OK, I went back and looked again to see what else I could do to remove
"unecessary changes", etc in gdg/infcmd.c. I took out the move of the
proceed call from the if/else branches to a single call after the
if/else statement. I moved the declarations to the top with the other
declarations. I moved the new if statement up a little. These changes
make the changes in the if/else are easier to read. I made a few other
cosmetic changes to get the diff to be as minimal as I could.

I hope this addresses your concerns. There are no functional changes.
I retested the patch. I will post an updated version of the patch.

Carl