New breakpoint_re_set call vs remote targets

New breakpoint_re_set call vs remote targets

[Daniel Jacobowitz]

This patch:

2009-06-17  Pierre Muller  <muller@ics.u-strasbg.fr>
        Pedro Alves  <pedro@codesourcery.com>

        * infcmd.c (post_create_inferior): Call breakpoint_re_set
	after target
        is pushed for watchpoint promotion to hardware watchpoint.

causes a testcase failure in nodebug.exp for arm-none-eabi.  It will
affect all bare-metal targets.

The sequence is "target remote", which calls post_create_inferior
before any program exists on the remote side.  Then later "load" fills
in the code.  So we're doing prologue skipping - by reading target
memory - before we've written the code to target memory.

I have long had a plan to speed up prologue skipping by making it read
directly from the executable if possible.  We're using the
executable's symbol table, so there's no reason to think the prologue
will have moved around on the target.  The problems with this approach
are (A) it involves changing a lot of symbol readers, and (B) I'm not
sure if we want to handle fix-and-continue style function patching in
which case we need to read from the target anyway.

Thoughts?  Any other approaches to fix this failure?

-- 
Daniel Jacobowitz
CodeSourcery

Re: New breakpoint_re_set call vs remote targets

[Pedro Alves]

On Wednesday 24 June 2009 20:03:46, Daniel Jacobowitz wrote:
> This patch:
> 
> 2009-06-17  Pierre Muller  <muller@ics.u-strasbg.fr>
>         Pedro Alves  <pedro@codesourcery.com>
> 
>         * infcmd.c (post_create_inferior): Call breakpoint_re_set
>         after target
>         is pushed for watchpoint promotion to hardware watchpoint.
> 
> causes a testcase failure in nodebug.exp for arm-none-eabi.  It will
> affect all bare-metal targets.
> 
> The sequence is "target remote", which calls post_create_inferior
> before any program exists on the remote side.  Then later "load" fills
> in the code.  So we're doing prologue skipping - by reading target
> memory - before we've written the code to target memory.

What does the failure look like?  I assume that either prologue
skipping just doesn't skip anything meaningful and the breakpoint
ends up set at the beginning of the garbage "prologue"; or, some exception
is thrown (I'd argue that prologue skipping shouldn't throw in this case).

This sounds like something that has always been broken, but we
didn't notice due to the fact that nothing happens pulls symbols
between "target remote" and "load" on those configurations: e.g.,
from your description, target remote; add-symbol-file; load;
should be failing even before that patch? (given that add-symbol-file
triggers a breakpoint reset).

> 
> I have long had a plan to speed up prologue skipping by making it read
> directly from the executable if possible.  We're using the
> executable's symbol table, so there's no reason to think the prologue
> will have moved around on the target.  The problems with this approach
> are (A) it involves changing a lot of symbol readers, 

Yeah, this could hacked by forcing trust-readonly-sections in
some places.  This and address spaces have made me think before that
to_xfer_partial may be a too stiff interface for memory reads
(in that we may end up reviving deprecated_xfer_memory...).

> and (B) I'm not 
> sure if we want to handle fix-and-continue style function patching in
> which case we need to read from the target anyway.

Right.  That's the main counter-point.

> Thoughts?  Any other approaches to fix this failure?

Assuming the debugging session didn't blow up, wouldn't a
breakpoint_re_set call after loading fix this?  The live
target has gotten new code loaded, so triggering
breakpoint re-evaluation and prologue skipping doesn't sound
wrong to me.

But, what was the real failure?

It also occurs to me that for always-inserted mode, we'd probably
want to uninsert breakpoints before loading.

-- 
Pedro Alves

Re: New breakpoint_re_set call vs remote targets

[Daniel Jacobowitz]

On Thu, Jun 25, 2009 at 12:37:16AM +0100, Pedro Alves wrote:
> What does the failure look like?  I assume that either prologue
> skipping just doesn't skip anything meaningful and the breakpoint
> ends up set at the beginning of the garbage "prologue"; or, some exception
> is thrown (I'd argue that prologue skipping shouldn't throw in this case).

The memory happens to be mostly zero at this point.  The zero pattern
in Thumb mode is lsl r0, #0 (from memory, don't shoot me if I'm
wrong).  That's a nop - which the prologue skipper happily skips.
In fact, in one case it keeps skipping right past the end of the
function.

(This is nodebug.exp.)

> This sounds like something that has always been broken, but we
> didn't notice due to the fact that nothing happens pulls symbols
> between "target remote" and "load" on those configurations: e.g.,
> from your description, target remote; add-symbol-file; load;
> should be failing even before that patch? (given that add-symbol-file
> triggers a breakpoint reset).

Probably so, but in practice it rarely ever triggered before now.
Now it happens all the time, although it's mitigated by debug info;
we'll use that for prologue skipping instead.

> Yeah, this could hacked by forcing trust-readonly-sections in
> some places.  This and address spaces have made me think before that
> to_xfer_partial may be a too stiff interface for memory reads
> (in that we may end up reviving deprecated_xfer_memory...).
> 
> > and (B) I'm not 
> > sure if we want to handle fix-and-continue style function patching in
> > which case we need to read from the target anyway.
> 
> Right.  That's the main counter-point.

Curious what folks think of the tradeoff here.

> > Thoughts?  Any other approaches to fix this failure?
> 
> Assuming the debugging session didn't blow up, wouldn't a
> breakpoint_re_set call after loading fix this?  The live
> target has gotten new code loaded, so triggering
> breakpoint re-evaluation and prologue skipping doesn't sound
> wrong to me.

Yeah, that would probably do it.  I think it's wacky that prologue
skipping relies on target memory, though.

> It also occurs to me that for always-inserted mode, we'd probably
> want to uninsert breakpoints before loading.

Uck.

-- 
Daniel Jacobowitz
CodeSourcery

Re: New breakpoint_re_set call vs remote targets

[Pedro Alves]

On Thursday 25 June 2009 00:46:34, Daniel Jacobowitz wrote:
> > Assuming the debugging session didn't blow up, wouldn't a
> > breakpoint_re_set call after loading fix this?  The live
> > target has gotten new code loaded, so triggering
> > breakpoint re-evaluation and prologue skipping doesn't sound
> > wrong to me.
> 
> Yeah, that would probably do it.  I think it's wacky that prologue
> skipping relies on target memory, though.
> 
> > It also occurs to me that for always-inserted mode, we'd probably
> > want to uninsert breakpoints before loading.
> 
> Uck.
> 

:-)  The way I tend to see it, is that a "load" is similar to
an execl, with the twist that a load may not replace the whole
address space --- I was going to mention mark_breakpoints_out
otherwise.

-- 
Pedro Alves

Re: New breakpoint_re_set call vs remote targets

[Doug Evans]

On Wed, Jun 24, 2009 at 12:03 PM, Daniel Jacobowitz<drow@false.org> wrote:
> This patch:
>
> 2009-06-17  Pierre Muller  <muller@ics.u-strasbg.fr>
>        Pedro Alves  <pedro@codesourcery.com>
>
>        * infcmd.c (post_create_inferior): Call breakpoint_re_set
>        after target
>        is pushed for watchpoint promotion to hardware watchpoint.
>
> causes a testcase failure in nodebug.exp for arm-none-eabi.  It will
> affect all bare-metal targets.
>
> The sequence is "target remote", which calls post_create_inferior
> before any program exists on the remote side.  Then later "load" fills
> in the code.  So we're doing prologue skipping - by reading target
> memory - before we've written the code to target memory.

"create_inferior" has a very specific connotation (at least in some
contexts), and at first glance it's odd that target_remote is calling
any foo_create_inferior.  [Consider, for example, that "run" uses
target_create_inferior, to_create_inferior is the target hook for
starting programs, and target remote doesn't support "run".]

The first question I had is why is target remote calling post_create_inferior?

So I go and look at post_create_inferior, which has this:

/* Common actions to take after creating any sort of inferior, by any
     means (running, attaching, connecting, et cetera).  The target
     should be stopped.  */

I wonder if name choices are making things harder than they should be.
[Harder in the sense that bugs get inadvertently introduced, and in
the sense that it's not as straightforward to reason about these
things.]
[Bad timing that this came up yesterday in a different context. :-)]

> I have long had a plan to speed up prologue skipping by making it read
> directly from the executable if possible.  We're using the
> executable's symbol table, so there's no reason to think the prologue
> will have moved around on the target.  The problems with this approach
> are (A) it involves changing a lot of symbol readers, and (B) I'm not
> sure if we want to handle fix-and-continue style function patching in
> which case we need to read from the target anyway.
>
> Thoughts?  Any other approaches to fix this failure?

I wonder if one useful step is to reassess post_create_inferior, and
maybe split it up or something.

From my perhaps ancient point of view, gdb is for debugging two kinds
of programs: hosted and freestanding (to borrow jargon from C - though
non-bare-metal and bare-metal may be more accurate. 1/2 :-)), and I
wonder if they're being inadvertently fused.
Or not.

Re: New breakpoint_re_set call vs remote targets

[Daniel Jacobowitz]

On Thu, Jun 25, 2009 at 09:52:36AM -0700, Doug Evans wrote:
> I wonder if one useful step is to reassess post_create_inferior, and
> maybe split it up or something.

Adding it was a huge boon for consolidation.  I don't really want to
split it up again if we don't have to.  Lots of actions used to be
missed when using a particular target, such as remote or remote-mips
or even core.

Converting watchpoints from hardware to software (the point of the new
call) is a sensible thing to do at the point of connection.  It's just
the prologue skipping that's bitten us - I like Pedro's idea of doing
this at load.

> From my perhaps ancient point of view, gdb is for debugging two kinds
> of programs: hosted and freestanding (to borrow jargon from C - though
> non-bare-metal and bare-metal may be more accurate. 1/2 :-)), and I
> wonder if they're being inadvertently fused.

I've been wondering about this too... I fear that if we introduce any
switch between these two modes, we'll find it's not granular enough.

-- 
Daniel Jacobowitz
CodeSourcery

Re: New breakpoint_re_set call vs remote targets

[Doug Evans]

On Thu, Jun 25, 2009 at 11:33 AM, Daniel Jacobowitz<drow@false.org> wrote:
> On Thu, Jun 25, 2009 at 09:52:36AM -0700, Doug Evans wrote:
>> From my perhaps ancient point of view, gdb is for debugging two kinds
>> of programs: hosted and freestanding (to borrow jargon from C - though
>> non-bare-metal and bare-metal may be more accurate. 1/2 :-)), and I
>> wonder if they're being inadvertently fused.
>
> I've been wondering about this too... I fear that if we introduce any
> switch between these two modes, we'll find it's not granular enough.

To me what one got depended on the target and how one used it.
I never found the need for an explicit switch.