[RFC/7.1] Reset breakpoints after load

[RFC/7.1] Reset breakpoints after load

[Daniel Jacobowitz]

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.

* 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.

* 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.

I'd like to do the long-term solution also, but it will take longer
than the 7.1 release.

-- 
Daniel Jacobowitz
CodeSourcery

2010-03-14  Daniel Jacobowitz  <dan@codesourcery.com>

	* symfile.c (generic_load): Reset breakpoints after loading.

---
 gdb/symfile.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: gdb-mainline/gdb/symfile.c
===================================================================
--- gdb-mainline.orig/gdb/symfile.c	2010-03-11 00:23:57.000000000 -0800
+++ gdb-mainline/gdb/symfile.c	2010-03-14 08:45:43.000000000 -0700
@@ -1890,6 +1890,16 @@ generic_load (char *args, int from_tty)
      for other targets too.  */
   regcache_write_pc (get_current_regcache (), entry);
 
+  /* Reset breakpoints.  This should not be necessary, but it is
+     needed because of two other factors.  One is that
+     post_create_inferior calls breakpoint_re_set, so breakpoints were
+     already reset when we connected to the target.  The other is that
+     prologue analyzers read memory from the target, even though they
+     rely on the symbol table too.  So when we reset breakpoints after
+     connection, we may have read uninitialized memory from the
+     target.  */
+  breakpoint_re_set ();
+
   /* FIXME: are we supposed to call symbol_file_add or not?  According
      to a comment from remote-mips.c (where a call to symbol_file_add
      was commented out), making the call confuses GDB if more than one

Re: [RFC/7.1] Reset breakpoints after load

[Joel Brobecker]

> * 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.

I like this solution best too, but ...

> 2010-03-14  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* symfile.c (generic_load): Reset breakpoints after loading.

... this seems fine, at least as a temporary measure.  The only drawback
I can see is performance, but I don't think it will be noticeable.

-- 
Joel

Re: [RFC/7.1] Reset breakpoints after load

[Pedro Alves]

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

Re: [RFC/7.1] Reset breakpoints after load

[Daniel Jacobowitz]

On Mon, Mar 15, 2010 at 07:27:05PM +0000, Pedro Alves wrote:
> > * 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.

Right.

> Given that I proposed exactly this at least a couple of times
> already,

I'd forgotten this, if I ever knew it :-)

> I'm not sure I agree in calling this a workaround though.

Your explanation is convincing.  I've rewritten the comment, and
checked it in.

> Wouldn't be such a stretch, although just
> 
>   - remove_breakpoints ();
> 
>    <do actual load>
> 
>   - breakpoint_re_set ();
> 
> would work too, and be simpler.

That seems like a good thing too... I'd support it, but I didn't
implement it today.

-- 
Daniel Jacobowitz
CodeSourcery

2010-03-16  Daniel Jacobowitz  <dan@codesourcery.com>

	* symfile.c (generic_load): Reset breakpoints after loading.

Index: symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.278
diff -u -p -r1.278 symfile.c
--- symfile.c	15 Mar 2010 09:31:34 -0000	1.278
+++ symfile.c	17 Mar 2010 00:03:09 -0000
@@ -1893,6 +1893,16 @@ generic_load (char *args, int from_tty)
      for other targets too.  */
   regcache_write_pc (get_current_regcache (), entry);
 
+  /* Reset breakpoints, now that we have changed the load image.  For
+     instance, breakpoints may have been set (or reset, by
+     post_create_inferior) while connected to the target but before we
+     loaded the program.  In that case, the prologue analyzer could
+     have read instructions from the target to find the right
+     breakpoint locations.  Loading has changed the contents of that
+     memory.  */
+
+  breakpoint_re_set ();
+
   /* FIXME: are we supposed to call symbol_file_add or not?  According
      to a comment from remote-mips.c (where a call to symbol_file_add
      was commented out), making the call confuses GDB if more than one

Re: [RFC/7.1] Reset breakpoints after load

[Pedro Alves]

On Wednesday 17 March 2010 00:04:23, Daniel Jacobowitz wrote:
> Your explanation is convincing.  I've rewritten the comment, and
> checked it in.

Thank you.

-- 
Pedro Alves