Re: [rfc] Fix PowerPC displaced stepping regression

Pedro Alves
2009-09-28T16:57:43Z

On Sunday 27 September 2009 22:47:13, Ulrich Weigand wrote:
> I wrote:
> > It seems this change broke displaced stepping on PowerPC.
> >
> > I'm not sure I understand the rationale behind these changes to the
> > displaced stepping logic in infrun.c in the first place. Why is
> > everything conditioned on gdbarch_software_single_step_p, which just
> > says whether or not the architecture has installed a single-stepping
> > routine -- but this alone doesn't say whether software stepping is
> > actually needed in any given situation ...
>
> OK, it seems there are two separate changes:
>
> - In non-stop mode, we never want to use software single-step as
> common code does not support this in multiple threads at once.

Right. Shouldn't we switch this particular predicate to
check the non_stop global instead?

> - On platforms with no hardware single-step available, GDB common
> code should not use "step" but "continue" to run displaced copies.

>
> The first change does make sense, also on PowerPC. It is in fact
> the second change that is problematic, as it would force PowerPC
> to implement a much more complex displaced stepping logic just to
> avoid using hardware single-stepping the displaced copies .. which
> there is no need for in the first place.
>
> The following patch keeps the first change, but makes the second
> change conditional on a new gdbarch callback instead of simply
> checking for gdbarch_software_single_step_p. This allows PowerPC
> to say that even though it has installed a SW single-step routine
> to handle some specific corner cases, it still wants to use HW
> stepping for displaced copies. The default is such that everything
> should be unchanged for the ARM case.

Did you consider making the gdbarch_displaced_step_copy_insn
callback itself return that it expects the target to be
continued instead of stepped? I see that it's
arm-tdep.c:arm_displaced_init_closure itself that inserts a breakpoint
after the relocated instructions. An original insn can be expanded
to more than one instruction, at displaced_step_copy time, so it
can be useful to say "continue" instead of several single-step
even if the target supported HW step, and this addresses the ppc/arm
issue as well.

So, displaced_step_prepare would propagate the "continue" vs
"step" up, and all its callers would do the old logic:

if (step)
{
if (gdbarch_software_single_step_p (gdbarch))
target_resume (ptid, 0, TARGET_SIGNAL_0);
else
target_resume (ptid, 1, TARGET_SIGNAL_0);
}
else
target_resume (ptid, 0, TARGET_SIGNAL_0);

... that is, we'd remove the checks for use_displaced_stepping from
maybe_software_singlestep, and use something like the
above in displaced_step_fixup, where we issue the target_resume
(with `step' being what gdbarch_displaced_step_copy_insn reported
it wanted).

--
Pedro Alves