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

Carl Love via Gdb-patches
2023-02-20T16:34:54Z

On Fri, 2023-02-17 at 12:24 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
>
> This looks generally OK to me, except for two minor issues
> in the code base, and one problem in the test suite:
>
>
> > diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> > index 77206fcbfe8..a65cc700fc9 100644
> > --- a/gdb/infcmd.c
> > +++ b/gdb/infcmd.c
> > @@ -1728,28 +1728,41 @@ finish_backward (struct finish_command_fsm
> > *sm)
> > no way that a function up the stack can have a return address
> > that's equal to its entry point. */
> >
> > - if (sal.pc != pc)
> > - {
> > - frame_info_ptr frame = get_selected_frame (nullptr);
> > - struct gdbarch *gdbarch = get_frame_arch (frame);
> > + CORE_ADDR alt_entry_point = sal.pc;
> > + CORE_ADDR entry_point = alt_entry_point;
> > + frame_info_ptr frame = get_selected_frame (nullptr);
> > + struct gdbarch *gdbarch = get_frame_arch (frame);
> >
> > - /* Set a step-resume at the function's entry point. Once
> > that's
> > - hit, we'll do one more step backwards. */
> > - symtab_and_line sr_sal;
> > - sr_sal.pc = sal.pc;
> > - sr_sal.pspace = get_frame_program_space (frame);
> > - insert_step_resume_breakpoint_at_sal (gdbarch,
> > - sr_sal, null_frame_id);
> > + if (gdbarch_skip_entrypoint_p (gdbarch))
> > + /* Some architectures, like PowerPC use local and global entry
> > points.
> > + There is only one Entry Point (GEP = LEP) for other
> > architectures.
> > + The GEP is an alternate entry point. The LEP is the normal
> > entry point.
> > + The value of entry_point was initialized to the alternate
> > entry point
> > + (GEP). It will be adjusted to the normal entry point if
> > the function
> > + has two entry points. */
> > + entry_point = gdbarch_skip_entrypoint (gdbarch, sal.pc);
> > +
> > + if ((pc >= alt_entry_point) && (pc <= entry_point))
> > + /* 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);
> > - }
> > 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 in the body of the function. Set a breakpoint to
> > backup to
> > + the normal entry point. */
> > + symtab_and_line sr_sal;
> > + 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);
> > }
>
> This change looks much larger than it actually is, because you've
> swapped the order of the if vs. else blocks (the orignal code sets
> the breakpoint in the if path and the single-step range in the
> else path, while you've swapped this). Can you swap this back to
> shorten the diff?

OK, swapped them.

>
> > + if (execution_direction == EXEC_REVERSE
> > + && ecs->event_thread->control.proceed_to_finish)
> > + {
> > + /* We are executing the reverse-finish command. */
> > + if (ecs->event_thread->stop_pc () >= ecs-
> > >stop_func_alt_start
> > + && ecs->event_thread->stop_pc () < ecs->stop_func_start
> > + && ecs->stop_func_alt_start != ecs->stop_func_start)
>
> This third condition seems redundant: if stop_pc is >= func_alt_start
> *and* < func_start, then func_alt_start cannot be equal func_start.

Yes, on Powerpc the third condition is redundant. However, on other
platforms, specifically X86, the third condition is always false which
ensures the if statement is false. This is important on the tests
gdb.btrace/tailcall-only.exp and gdb.btrace/tailcall.exp which only run
on X86. The tests call the up command which will satisfy conditions 1
and 2 and thus do an "extra" step command resulting in a test failure.
The third condition ensures that we never execute an additional
reverse-step unless the platform supports multiple entry points and we
are at the alternate entry point. I can see where this is not obvious
at first glance. I added a comment to the if statement explaining the
need for the apparent redundancy.

Also, the if statement is nested inside of the if reverse if statement.
I combined the two if statements into a single if statement.

> > diff --git a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > index 94292d5eb9b..dc5cf097511 100644
> > --- a/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > +++ b/gdb/testsuite/gdb.reverse/step-indirect-call-thunk.exp
> > @@ -36,39 +36,6 @@ if { ![runto_main] } {
> > return -1
> > }
> >
> > -# Do repeated stepping COMMANDs in order to reach TARGET from
> > CURRENT
> > -#
> > -# COMMAND is a stepping command
> > -# CURRENT is a string matching the current location
> > -# TARGET is a string matching the target location
> > -# TEST is the test name
> > -#
> > -# The function issues repeated COMMANDs as long as the location
> > matches
> > -# CURRENT up to a maximum of 100 steps.
> > -#
> > -# TEST passes if the resulting location matches TARGET and fails
> > -# otherwise.
> > -#
> > -proc step_until { command current target test } {
> > - global gdb_prompt
> > -
> > - set count 0
> > - gdb_test_multiple "$command" "$test" {
> > - -re "$current.*$gdb_prompt $" {
> > - incr count
> > - if { $count < 100 } {
> > - send_gdb "$command\n"
> > - exp_continue
> > - } else {
> > - fail "$test"
> > - }
> > - }
> > - -re "$target.*$gdb_prompt $" {
> > - pass "$test"
> > - }
> > - }
> > -}
> > -
> > gdb_test_no_output "record"
> > gdb_test "next" ".*" "record trace"
>
> How is it OK to just remove this procedure? This is still used
> in the rest of the step-indirect-call-thunk.exp test case ...

Yes, looks like my fix up of the call to step_until in step-indirect-
call-thunk.exp got lost somewhere along the line. I remember making
the change.

I went back to see why the regression testing didn't catch the error.
On PowerPC, the test doesn't run as the compile options "-mindirect-
branch=thunk" and "-mfunction-return=thunk" are not supported on
PowerPC, so the test fails anyways. It looks like the test didn't run
on my X86 either due to the regression test failing before getting to
the gdb tests. Looks like I need to run the regression test in
subdirectory gdb to make sure all of the gdb tests get run.

I have run the updated patch, on X86, manually and in the regression
test to make sure there are no regressions with this test. I checked
out the base and regression runs on X86 to make sure the regression
tests are all be run.

>
> I get why you may want to reduce duplication, but then you'd
> have to update current users of "step_until" as well.

Yes, those updates got lost rebasing the patch at some point. Fixed.

>
> Also, talking about duplication, should the (already separate)
> gdb_step_until command in lib/gdb.exp now just be a variant
> of the new repeat_cmd_until ?

OK, I extended the new repeat_cmd_until proceedure to take the
additional input max_steps. I created a new version of gdb_step_until
that calls repeat_cmd_until. I added an optional argument, current, to
gdb_step_until proceedure for the current line. By default, current is
set to "\}" which is needed by the call to repeat_cmd_until proceedure.
The new gdb_step_until proceedure works in the existing regression
tests without changing the gdb_step_until call in the existing tests.

Thanks for reviewing the patch. I will post an updated patch shortly.

Carl