Re: [RFC/7.1] Reset breakpoints after load

[Pedro Alves <pedro@codesourcery.com>]

On Sunday 14 March 2010 16:21:07, Daniel Jacobowitz wrote:
> Since 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.
> 
> GDB performs this sequence:
> 
> % gdb -quiet file
> (gdb) break main
> [Breakpoint set after prologue]
> (gdb) target remote :PORT
> [Connect to remote target]
> [breakpoint_re_set called]
> (gdb) load
> (gdb) continue
> 
> If the prologue skipping logic reads from memory, then when
> breakpoint_re_set is called, it will read garbage.  Many of the
> prologue analyzers do, although the effect is mitigated by
> skip_prologue_using_sal, which is used in preference if possible.
> 
> I believe we worked around this bug locally for MIPS.  I've also just
> encountered it while testing a patch for ARM that changes the prologue
> skipping behavior.
> 
> I can think of three solutions.
> 
> * Don't reset breakpoints here.  Promote watchpoints and make no other
> changes.  A bit twisty to implement, unfortunately.

This would only papers over the issue.  Imagine that the patch
that introduced the new breakpoint_re_set call was reverted.  You
can still trigger the issue at hand easily.  E.g.:

 $ gdb
 (gdb) target remote :PORT
 [Connect to remote target]
 (gdb) file FILE
 [breakpoint_re_set called]
 (gdb) load
 (gdb) continue

> 
> * Don't read from the target during prologue analyzers; only read from
> the executable file.  I like this solution best, and it has other
> merits (it's faster!).  But it's the most work.

"only" would be too strong.  You'd want "prefer", like
trust-readonly-sections.  We may have debug info available but
no pure memory contents to read from.

> * The easy solution: Reset breakpoints again once we know that target
> memory is valid.
> 
> Any comments on this patch?  It has no effect on test results on
> arm-none-eabi today, and fixes two hundred or so failures with another
> patch that required reading from the target during prologue analysis.

Given that I proposed exactly this at least a couple of times
already, I don't expect you to be waiting for me to say go
ahead.  :-)  I'm not sure I agree in calling this a workaround
though.  IMO, this situation is analog to an "exec".
The previous memory image is supposedly replaced by
"load".  Consider breakpoints always-inserted mode:  the
previous traps are simply overwriten by "load" behind the
breakpoint's module.  Calling:

 - mark_breakpoints_out ();
 - update_breakpoints_after_exec ();
 - breakpoint_re_set ();

Wouldn't be such a stretch, although just

  - remove_breakpoints ();

   <do actual load>

  - breakpoint_re_set ();

would work too, and be simpler.

The comment reads a bit like the post_create_inferior
path is the only that causes this, but as shown in the example abov
, any breakpoint_re_set call would trigger the issue, so maybe
I'd rephrase it a bit in that direction.

IMO.

-- 
Pedro Alves