[patch] Simplify breakpoint.c function parameters

[patch] Simplify breakpoint.c function parameters

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 409 bytes --]

Hi,

the patch has no effect on the functionality but I find the simplified code
less magic to understand.

It fixes a small bug - update_watchpoint() for bp_watchpoint (software
watchpoint) created breakpoint locations bp_loc_hardware_watchpoint while it
should be bp_loc_other.  But I am not aware of any (possibly negative) effect
it had.

No regressions on {x86_64,ia64}-unknown-linux-gnu.


Regards,
Jan

[-- Attachment #2: gdb-breakpoint-type-simplify.patch --]
[-- Type: text/plain, Size: 3582 bytes --]

2008-12-07  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix loc_type of `bp_location's created by update_watchpoint.
	* breakpoint.c (allocate_bp_location): Remove the bp_type parameter.
	Replace bp_type by bpt->type.  Update prototype.  All callers updated.
	(add_location_to_breakpoint):  Remove the bp_type parameter.
	Replace bp_type by b->type.  All callers updated.
	(set_breakpoint_location_function): Replace bptype by b->type.

--- gdb/breakpoint.c	7 Dec 2008 15:59:51 -0000	1.364
+++ gdb/breakpoint.c	7 Dec 2008 19:19:40 -0000
@@ -181,8 +181,7 @@ static int single_step_breakpoint_insert
 
 static void free_bp_location (struct bp_location *loc);
 
-static struct bp_location *
-allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
+static struct bp_location *allocate_bp_location (struct breakpoint *bpt);
 
 static void update_global_location_list (int);
 
@@ -924,7 +923,7 @@ update_watchpoint (struct breakpoint *b,
 		  else if (b->type == bp_access_watchpoint)
 		    type = hw_access;
 		  
-		  loc = allocate_bp_location (b, bp_hardware_watchpoint);
+		  loc = allocate_bp_location (b);
 		  for (tmp = &(b->loc); *tmp != NULL; tmp = &((*tmp)->next))
 		    ;
 		  *tmp = loc;
@@ -4013,7 +4044,7 @@ adjust_breakpoint_address (CORE_ADDR bpa
 /* Allocate a struct bp_location.  */
 
 static struct bp_location *
-allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type)
+allocate_bp_location (struct breakpoint *bpt)
 {
   struct bp_location *loc, *loc_p;
 
@@ -4025,7 +4056,7 @@ allocate_bp_location (struct breakpoint 
   loc->shlib_disabled = 0;
   loc->enabled = 1;
 
-  switch (bp_type)
+  switch (bpt->type)
     {
     case bp_breakpoint:
     case bp_until:
@@ -4144,9 +4175,9 @@ set_breakpoint_location_function (struct
      breakpoint may cause target_read_memory() 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);
+  adjusted_address = adjust_breakpoint_address (sal.pc, b->type);
 
-  b->loc = allocate_bp_location (b, bptype);
+  b->loc = allocate_bp_location (b);
   b->loc->requested_address = sal.pc;
   b->loc->address = adjusted_address;
 
@@ -4981,18 +5015,17 @@ mention (struct breakpoint *b)
 \f
 
 static struct bp_location *
-add_location_to_breakpoint (struct breakpoint *b, enum bptype bptype,
+add_location_to_breakpoint (struct breakpoint *b,
 			    const struct symtab_and_line *sal)
 {
   struct bp_location *loc, **tmp;
 
-  loc = allocate_bp_location (b, bptype);
+  loc = allocate_bp_location (b);
   for (tmp = &(b->loc); *tmp != NULL; tmp = &((*tmp)->next))
     ;
   *tmp = loc;
   loc->requested_address = sal->pc;
-  loc->address = adjust_breakpoint_address (loc->requested_address,
-					    bptype);
+  loc->address = adjust_breakpoint_address (loc->requested_address, b->type);
   loc->section = sal->section;
 
   set_breakpoint_location_function (loc);
@@ -5090,7 +5125,7 @@ create_breakpoint (struct symtabs_and_li
 	}
       else
 	{
-	  loc = add_location_to_breakpoint (b, type, &sal);
+	  loc = add_location_to_breakpoint (b, &sal);
 	}
 
       if (bp_loc_is_permanent (loc))
@@ -7226,7 +7263,7 @@ update_breakpoint_locations (struct brea
   for (i = 0; i < sals.nelts; ++i)
     {
       struct bp_location *new_loc = 
-	add_location_to_breakpoint (b, b->type, &(sals.sals[i]));
+	add_location_to_breakpoint (b, &(sals.sals[i]));
 
       /* Reparse conditions, they might contain references to the
 	 old symtab.  */

Re: [patch] Simplify breakpoint.c function parameters

[Vladimir Prus]

Jan Kratochvil wrote:

> Hi,
> 
> the patch has no effect on the functionality but I find the simplified code
> less magic to understand.
> 
> It fixes a small bug - update_watchpoint() for bp_watchpoint (software
> watchpoint) created breakpoint locations bp_loc_hardware_watchpoint 

update_watchpoint() is documented thusly:

        /* Assuming that B is a hardware watchpoint:
           - Reparse watchpoint expression, is REPARSE is non-zero
           - Evaluate expression and store the result in B->val
           - Update the list of values that must be watched in B->loc.

           If the watchpoint is disabled, do nothing.  If this is
           local watchpoint that is out of scope, delete it.  */
        static void
        update_watchpoint (struct breakpoint *b, int reparse)

but it appears to be actually called from breakpoint_re_set_one for the
software watchpoint, too. I suspect it's a bug -- for software watchpoint
we don't have to get a list of values to watch, which update_watchpoint()
does more or less unconditionally. But this bug might be benign -- we probably
never actually insert the computed locations.

> while it 
> should be bp_loc_other.  But I am not aware of any (possibly negative) effect
> it had.
> 
> No regressions on {x86_64,ia64}-unknown-linux-gnu.

FWIW, this sounds reasonable and safe to me.

- Volodya

Re: [patch] Simplify breakpoint.c function parameters

[Jan Kratochvil]

On Mon, 08 Dec 2008 08:34:22 +0100, Vladimir Prus wrote:
> FWIW, this sounds reasonable and safe to me.

On Mon, 08 Dec 2008 12:05:36 +0100, Joel Brobecker wrote:
> After careful review, this looks right to me.

Thanks both for the review, checked it in.


> I'm wondering if it would make sense to get rid of the loc_type field
> and compute it on the fly through a function...

I do not disagree.


Regards,
Jan

Re: [patch] Simplify breakpoint.c function parameters

[Joel Brobecker]

> It fixes a small bug - update_watchpoint() for bp_watchpoint (software
> watchpoint) created breakpoint locations bp_loc_hardware_watchpoint while it
> should be bp_loc_other.  But I am not aware of any (possibly negative) effect
> it had.

That's true!

> 2008-12-07  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Fix loc_type of `bp_location's created by update_watchpoint.
> 	* breakpoint.c (allocate_bp_location): Remove the bp_type parameter.
> 	Replace bp_type by bpt->type.  Update prototype.  All callers updated.
> 	(add_location_to_breakpoint):  Remove the bp_type parameter.
> 	Replace bp_type by b->type.  All callers updated.
> 	(set_breakpoint_location_function): Replace bptype by b->type.

After careful review, this looks right to me.

I'm wondering if it would make sense to get rid of the loc_type field
and compute it on the fly through a function...

-- 
Joel