[rfc] Set a breakpoint's type before adjusting its address

[rfc] Set a breakpoint's type before adjusting its address

[Kevin Buettner]

At the moment, running GDB on an FR-V target will generate lots of
"reading through apparently deleted breakpoint" warnings.  This is due
to the fact that (due to architectural constraints) FR-V needs to
adjust breakpoint addresses.  When it does so, it reads through
read_memory_nobpt(), triggering the warning due to the fact that the
breakpoint's type hasn't been set yet.  The patch below moves the type
assignment prior to the adjustment thus avoiding this situation.

Comments?

	* breakpoint.c (set_raw_breakpoint): Set breakpoint type before
	attempting to adjust its address.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.244
diff -u -p -r1.244 breakpoint.c
--- gdb/breakpoint.c	10 Apr 2007 14:53:46 -0000	1.244
+++ gdb/breakpoint.c	27 Apr 2007 22:06:44 -0000
@@ -4213,6 +4213,14 @@ set_raw_breakpoint (struct symtab_and_li
 
   b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
   memset (b, 0, sizeof (*b));
+
+  /* Set type before performing breakpoint adjustment.  If the breakpoint
+     needs to be adjusted, it's possible that this raw breakpoint will be
+     read by read_memory_nobpt().  If the type is unset (zero), the
+     "reading through apparently deleted breakpoint" warning will be
+     triggered.  */
+  b->type = bptype;
+
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
   b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
@@ -4223,7 +4231,6 @@ set_raw_breakpoint (struct symtab_and_li
     b->source_file = savestring (sal.symtab->filename,
 				 strlen (sal.symtab->filename));
   b->loc->section = sal.section;
-  b->type = bptype;
   b->language = current_language->la_language;
   b->input_radix = input_radix;
   b->thread = -1;

Re: [rfc] Set a breakpoint's type before adjusting its address

[Daniel Jacobowitz]

On Fri, Apr 27, 2007 at 03:15:59PM -0700, Kevin Buettner wrote:
> At the moment, running GDB on an FR-V target will generate lots of
> "reading through apparently deleted breakpoint" warnings.  This is due
> to the fact that (due to architectural constraints) FR-V needs to
> adjust breakpoint addresses.  When it does so, it reads through
> read_memory_nobpt(), triggering the warning due to the fact that the
> breakpoint's type hasn't been set yet.  The patch below moves the type
> assignment prior to the adjustment thus avoiding this situation.
> 
> Comments?
> 
> 	* breakpoint.c (set_raw_breakpoint): Set breakpoint type before
> 	attempting to adjust its address.

This happens because the breakpoint's location is already on the
location chain, right?  Alternatively, we could move that from the end
of allocate_bp_location to the end of set_raw_breakpoint, and avoid
the inconsistency.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Set a breakpoint's type before adjusting its address

[Kevin Buettner]

On Sat, 28 Apr 2007 17:45:10 -0400
Daniel Jacobowitz <drow@false.org> wrote:

> This happens because the breakpoint's location is already on the
> location chain, right?

Right.

> Alternatively, we could move that from the end
> of allocate_bp_location to the end of set_raw_breakpoint, and avoid
> the inconsistency.

If we can do it, I think it'd be nice to keep the code which allocates
the location together with the code which adds the newly allocated
location to the chain.

I agree that my earlier patch is not very nice in that
adjust_breakpoint_address() was being called with an only partially
initialized location on the location chain.  That patch was a band-aid
in that it initialized those bits which a particular function
(read_memory_nobpt) cared about, but who knows what else might break
if some other function were called.

Appended below is a new patch which calls adjust_breakpoint_address()
prior to allocating the breakoint's location.  What do you think of
this approach?

Kevin

	* breakpoint.c (set_raw_breakpoint): Adjust breakpoint's address
	prior to allocating its location.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.246
diff -u -p -r1.246 breakpoint.c
--- breakpoint.c	13 Apr 2007 13:50:32 -0000	1.246
+++ breakpoint.c	2 May 2007 23:57:06 -0000
@@ -4189,13 +4189,23 @@ struct breakpoint *
 set_raw_breakpoint (struct symtab_and_line sal, enum bptype bptype)
 {
   struct breakpoint *b, *b1;
+  CORE_ADDR adjusted_address;
 
   b = (struct breakpoint *) xmalloc (sizeof (struct breakpoint));
   memset (b, 0, sizeof (*b));
+
+  /* Adjust the breakpoint's address prior to allocating a location.
+     Once we call allocate_bp_location(), that mostly uninitialized
+     location will be placed on the location chain.  Adjustment of the
+     breakpoint may cause read_memory_nobpt() to be called and we do
+     not want its scan of the location chain to find a breakpoint and
+     location that's only been partially initialized.  */
+  adjusted_address = adjust_breakpoint_address (sal.pc, bptype);
+
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
-  b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
-                                               bptype);
+  b->loc->address = adjusted_address;
+
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else

Re: [rfc] Set a breakpoint's type before adjusting its address

[Daniel Jacobowitz]

On Wed, May 02, 2007 at 05:15:49PM -0700, Kevin Buettner wrote:
> Appended below is a new patch which calls adjust_breakpoint_address()
> prior to allocating the breakoint's location.  What do you think of
> this approach?

I'm much happier with this patch; I think it's OK to commit.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Set a breakpoint's type before adjusting its address

[Kevin Buettner]

On Wed, 2 May 2007 22:38:49 -0400
Daniel Jacobowitz <drow@false.org> wrote:

> On Wed, May 02, 2007 at 05:15:49PM -0700, Kevin Buettner wrote:
> > Appended below is a new patch which calls adjust_breakpoint_address()
> > prior to allocating the breakoint's location.  What do you think of
> > this approach?
> 
> I'm much happier with this patch; I think it's OK to commit.

Thanks for looking it over.  I've committed it.

Kevin