Re: [rfc] Fix PowerPC displaced stepping regression

[Pedro Alves <pedro@codesourcery.com>]

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