[RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

In the course of running the GDB testsuite on FR-V, I discovered that it's
a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
watchpoint ends up at the wrong address.  The patch below simply disables
adjustment for watchpoints and some eventpoints.

Any comments on this patch?  If not, I'll commit it towards the end of the
week.

Kevin

	* breakpoint.c (adjust_breakpoint_address): Don't adjust
	breakpoint address for watchpoints or the catch eventpoints.
	Add new paramter bptype.  Adjust all callers.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.162
diff -u -p -r1.162 breakpoint.c
--- breakpoint.c	28 Feb 2004 16:56:12 -0000	1.162
+++ breakpoint.c	15 Mar 2004 20:18:45 -0000
@@ -99,7 +99,8 @@ static void check_duplicates (struct bre
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
-static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr);
+static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr,
+                                            enum bptype bptype);
 
 static void describe_other_breakpoints (CORE_ADDR, asection *);
 
@@ -3947,13 +3948,25 @@ breakpoint_adjustment_warning (CORE_ADDR
    this function is simply the identity function.  */
 
 static CORE_ADDR
-adjust_breakpoint_address (CORE_ADDR bpaddr)
+adjust_breakpoint_address (CORE_ADDR bpaddr, enum bptype bptype)
 {
   if (!gdbarch_adjust_breakpoint_address_p (current_gdbarch))
     {
       /* Very few targets need any kind of breakpoint adjustment.  */
       return bpaddr;
     }
+  else if (bptype == bp_watchpoint
+           || bptype == bp_hardware_watchpoint
+           || bptype == bp_read_watchpoint
+           || bptype == bp_access_watchpoint
+           || bptype == bp_catch_fork
+           || bptype == bp_catch_vfork
+           || bptype == bp_catch_exec)
+    {
+      /* Watchpoints and the various bp_catch_* eventpoints should not
+         have their addresses modified.  */
+      return bpaddr;
+    }
   else
     {
       CORE_ADDR adjusted_bpaddr;
@@ -4062,7 +4075,8 @@ set_raw_breakpoint (struct symtab_and_li
   memset (b, 0, sizeof (*b));
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
-  b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+  b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                               bptype);
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else
@@ -4622,7 +4636,8 @@ set_longjmp_resume_breakpoint (CORE_ADDR
     if (b->type == bp_longjmp_resume)
     {
       b->loc->requested_address = pc;
-      b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+      b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                                   b->type);
       b->enable_state = bp_enabled;
       b->frame_id = frame_id;
       check_duplicates (b);
@@ -5879,7 +5894,8 @@ watch_command_1 (char *arg, int accessfl
 	  scope_breakpoint->loc->requested_address
 	    = get_frame_pc (prev_frame);
 	  scope_breakpoint->loc->address
-	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address);
+	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
+	                                 scope_breakpoint->type);
 
 	  /* The scope breakpoint is related to the watchpoint.  We
 	     will need to act on them together.  */
@@ -5894,11 +5910,6 @@ watch_command_1 (char *arg, int accessfl
    in hardware.  If the watchpoint can not be handled
    in hardware return zero.  */
 
-#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
-    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
-#endif
-
 #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
      (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
@@ -7190,7 +7201,8 @@ breakpoint_re_set_one (void *bint)
 	      b->line_number = sals.sals[i].line;
 	      b->loc->requested_address = sals.sals[i].pc;
 	      b->loc->address
-	        = adjust_breakpoint_address (b->loc->requested_address);
+	        = adjust_breakpoint_address (b->loc->requested_address,
+		                             b->type);
 
 	      /* Used to check for duplicates here, but that can
 	         cause trouble, as it doesn't check for disabled

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Eli Zaretskii]

> Date: Mon, 15 Mar 2004 13:30:02 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> 
> In the course of running the GDB testsuite on FR-V, I discovered that it's
> a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
> watchpoint ends up at the wrong address.  The patch below simply disables
> adjustment for watchpoints and some eventpoints.

I'd like to hear more details.  When do we need to adjust the
breakpoint address, for starters?

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

On Mon, 15 Mar 2004 23:05:13 +0200
"Eli Zaretskii" <eliz@elta.co.il> wrote:

> > Date: Mon, 15 Mar 2004 13:30:02 -0700
> > From: Kevin Buettner <kevinb@redhat.com>
> > 
> > In the course of running the GDB testsuite on FR-V, I discovered that it's
> > a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
> > watchpoint ends up at the wrong address.  The patch below simply disables
> > adjustment for watchpoints and some eventpoints.
> 
> I'd like to hear more details.  When do we need to adjust the
> breakpoint address, for starters?

We have some documentation about this in gdbint.texinfo:

    The FR-V target (see @file{frv-tdep.c}) requires this method.
    The FR-V is a VLIW architecture in which a number of RISC-like
    instructions are grouped (packed) together into an aggregate
    instruction or instruction bundle.  When the processor executes
    one of these bundles, the component instructions are executed
    in parallel.

    In the course of optimization, the compiler may group instructions
    from distinct source statements into the same bundle.  The line number
    information associated with one of the latter statements will likely
    refer to some instruction other than the first one in the bundle.  So,
    if the user attempts to place a breakpoint on one of these latter
    statements, @value{GDBN} must be careful to @emph{not} place the break
    instruction on any instruction other than the first one in the bundle.
    (Remember though that the instructions within a bundle execute
    in parallel, so the @emph{first} instruction is the instruction
    at the lowest address and has nothing to do with execution order.)

    The FR-V's @code{ADJUST_BREAKPOINT_ADDRESS} method will adjust a
    breakpoint's address by scanning backwards for the beginning of
    the bundle, returning the address of the bundle.

So, on FR-V, we were scanning backwards from the position of a
watchpoint, looking for the start of a VLIW instruction.  Given that
watchpoints are usually set on data, this makes very little sense. 
Even when it might (sort of) make sense, we don't want to do any
adjustment.  E.g, if we were watching a specific executable location -
perhaps it was being mysteriously modified - we wouldn't want to
adjust the location of the watchpoint.  The reason is that we're
viewing that executable location as data.  We want to be informed
when that location changes, not some location (which represents the
start of the VLIW location) before it.

To make this more concise, it makes sense to adjust breakpoint
addresses, because the architectural constraint is related to the
placment of breakpoints in executable code.  It doesn't make sense to
do this kind of adjustment on watchpoints since watchpoints are
concerned with data related accesses.

The other client of TARGET_ADJUST_BREAKPOINT is PPC.  It uses the
breakpoint adjustment mechanism to dereference function descriptors.
I think my proposed change makes sense for this case as well.  I.e,
we don't want to prevent the user from putting a watchpoint on a
function descriptor.  As things stand now, that is what would happen.

Kevin

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Eli Zaretskii]

> Date: Mon, 15 Mar 2004 14:35:49 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> > 
> > I'd like to hear more details.  When do we need to adjust the
> > breakpoint address, for starters?
> 
> We have some documentation about this in gdbint.texinfo:
> 
>     The FR-V target (see @file{frv-tdep.c}) requires this method.

Ah, yes, thanks for reminding me about that, and for explaining the
rationale for the patch.  I have no objections for it to go in.

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Eli Zaretskii]

> Date: Mon, 15 Mar 2004 14:35:49 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> > 
> > I'd like to hear more details.  When do we need to adjust the
> > breakpoint address, for starters?
> 
> We have some documentation about this in gdbint.texinfo:
> 
>     The FR-V target (see @file{frv-tdep.c}) requires this method.

Ah, yes, thanks for reminding me about that, and for explaining the
rationale for the patch.  I have no objections for it to go in.

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

On Mon, 15 Mar 2004 23:05:13 +0200
"Eli Zaretskii" <eliz@elta.co.il> wrote:

> > Date: Mon, 15 Mar 2004 13:30:02 -0700
> > From: Kevin Buettner <kevinb@redhat.com>
> > 
> > In the course of running the GDB testsuite on FR-V, I discovered that it's
> > a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
> > watchpoint ends up at the wrong address.  The patch below simply disables
> > adjustment for watchpoints and some eventpoints.
> 
> I'd like to hear more details.  When do we need to adjust the
> breakpoint address, for starters?

We have some documentation about this in gdbint.texinfo:

    The FR-V target (see @file{frv-tdep.c}) requires this method.
    The FR-V is a VLIW architecture in which a number of RISC-like
    instructions are grouped (packed) together into an aggregate
    instruction or instruction bundle.  When the processor executes
    one of these bundles, the component instructions are executed
    in parallel.

    In the course of optimization, the compiler may group instructions
    from distinct source statements into the same bundle.  The line number
    information associated with one of the latter statements will likely
    refer to some instruction other than the first one in the bundle.  So,
    if the user attempts to place a breakpoint on one of these latter
    statements, @value{GDBN} must be careful to @emph{not} place the break
    instruction on any instruction other than the first one in the bundle.
    (Remember though that the instructions within a bundle execute
    in parallel, so the @emph{first} instruction is the instruction
    at the lowest address and has nothing to do with execution order.)

    The FR-V's @code{ADJUST_BREAKPOINT_ADDRESS} method will adjust a
    breakpoint's address by scanning backwards for the beginning of
    the bundle, returning the address of the bundle.

So, on FR-V, we were scanning backwards from the position of a
watchpoint, looking for the start of a VLIW instruction.  Given that
watchpoints are usually set on data, this makes very little sense. 
Even when it might (sort of) make sense, we don't want to do any
adjustment.  E.g, if we were watching a specific executable location -
perhaps it was being mysteriously modified - we wouldn't want to
adjust the location of the watchpoint.  The reason is that we're
viewing that executable location as data.  We want to be informed
when that location changes, not some location (which represents the
start of the VLIW location) before it.

To make this more concise, it makes sense to adjust breakpoint
addresses, because the architectural constraint is related to the
placment of breakpoints in executable code.  It doesn't make sense to
do this kind of adjustment on watchpoints since watchpoints are
concerned with data related accesses.

The other client of TARGET_ADJUST_BREAKPOINT is PPC.  It uses the
breakpoint adjustment mechanism to dereference function descriptors.
I think my proposed change makes sense for this case as well.  I.e,
we don't want to prevent the user from putting a watchpoint on a
function descriptor.  As things stand now, that is what would happen.

Kevin

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Eli Zaretskii]

> Date: Mon, 15 Mar 2004 13:30:02 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> 
> In the course of running the GDB testsuite on FR-V, I discovered that it's
> a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
> watchpoint ends up at the wrong address.  The patch below simply disables
> adjustment for watchpoints and some eventpoints.

I'd like to hear more details.  When do we need to adjust the
breakpoint address, for starters?

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Andrew Cagney]

> 
> 	* breakpoint.c (adjust_breakpoint_address): Don't adjust
> 	breakpoint address for watchpoints or the catch eventpoints.
> 	Add new paramter bptype.  Adjust all callers.

Is this intended?

> -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> -    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
> -#endif
> -

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Andrew Cagney]

> 
> 	* breakpoint.c (adjust_breakpoint_address): Don't adjust
> 	breakpoint address for watchpoints or the catch eventpoints.
> 	Add new paramter bptype.  Adjust all callers.

Is this intended?

> -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> -    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
> -#endif
> -

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

On Mon, 15 Mar 2004 15:40:43 -0500
Andrew Cagney <cagney@gnu.org> wrote:

> > 	* breakpoint.c (adjust_breakpoint_address): Don't adjust
> > 	breakpoint address for watchpoints or the catch eventpoints.
> > 	Add new paramter bptype.  Adjust all callers.
> 
> Is this intended?
> 
> > -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> > -    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
> > -#endif
> > -
> 

Oops!  Good catch.  That bit was not intended for this patch.  I'll
submit that separately as an obvious fix.  (Since the macro in question
is now defined in target.h.)

Here's the patch again with that bit removed...

	* breakpoint.c (adjust_breakpoint_address): Don't adjust
	breakpoint address for watchpoints or the catch eventpoints.
	Add new paramter bptype.  Adjust all callers.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.162
diff -u -p -r1.162 breakpoint.c
--- breakpoint.c	28 Feb 2004 16:56:12 -0000	1.162
+++ breakpoint.c	15 Mar 2004 21:12:45 -0000
@@ -99,7 +99,8 @@ static void check_duplicates (struct bre
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
-static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr);
+static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr,
+                                            enum bptype bptype);
 
 static void describe_other_breakpoints (CORE_ADDR, asection *);
 
@@ -3947,13 +3948,25 @@ breakpoint_adjustment_warning (CORE_ADDR
    this function is simply the identity function.  */
 
 static CORE_ADDR
-adjust_breakpoint_address (CORE_ADDR bpaddr)
+adjust_breakpoint_address (CORE_ADDR bpaddr, enum bptype bptype)
 {
   if (!gdbarch_adjust_breakpoint_address_p (current_gdbarch))
     {
       /* Very few targets need any kind of breakpoint adjustment.  */
       return bpaddr;
     }
+  else if (bptype == bp_watchpoint
+           || bptype == bp_hardware_watchpoint
+           || bptype == bp_read_watchpoint
+           || bptype == bp_access_watchpoint
+           || bptype == bp_catch_fork
+           || bptype == bp_catch_vfork
+           || bptype == bp_catch_exec)
+    {
+      /* Watchpoints and the various bp_catch_* eventpoints should not
+         have their addresses modified.  */
+      return bpaddr;
+    }
   else
     {
       CORE_ADDR adjusted_bpaddr;
@@ -4062,7 +4075,8 @@ set_raw_breakpoint (struct symtab_and_li
   memset (b, 0, sizeof (*b));
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
-  b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+  b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                               bptype);
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else
@@ -4622,7 +4636,8 @@ set_longjmp_resume_breakpoint (CORE_ADDR
     if (b->type == bp_longjmp_resume)
     {
       b->loc->requested_address = pc;
-      b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+      b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                                   b->type);
       b->enable_state = bp_enabled;
       b->frame_id = frame_id;
       check_duplicates (b);
@@ -5879,7 +5894,8 @@ watch_command_1 (char *arg, int accessfl
 	  scope_breakpoint->loc->requested_address
 	    = get_frame_pc (prev_frame);
 	  scope_breakpoint->loc->address
-	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address);
+	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
+	                                 scope_breakpoint->type);
 
 	  /* The scope breakpoint is related to the watchpoint.  We
 	     will need to act on them together.  */
@@ -7190,7 +7206,8 @@ breakpoint_re_set_one (void *bint)
 	      b->line_number = sals.sals[i].line;
 	      b->loc->requested_address = sals.sals[i].pc;
 	      b->loc->address
-	        = adjust_breakpoint_address (b->loc->requested_address);
+	        = adjust_breakpoint_address (b->loc->requested_address,
+		                             b->type);
 
 	      /* Used to check for duplicates here, but that can
 	         cause trouble, as it doesn't check for disabled

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

On Mon, 15 Mar 2004 15:40:43 -0500
Andrew Cagney <cagney@gnu.org> wrote:

> > 	* breakpoint.c (adjust_breakpoint_address): Don't adjust
> > 	breakpoint address for watchpoints or the catch eventpoints.
> > 	Add new paramter bptype.  Adjust all callers.
> 
> Is this intended?
> 
> > -#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
> > -#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
> > -    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
> > -#endif
> > -
> 

Oops!  Good catch.  That bit was not intended for this patch.  I'll
submit that separately as an obvious fix.  (Since the macro in question
is now defined in target.h.)

Here's the patch again with that bit removed...

	* breakpoint.c (adjust_breakpoint_address): Don't adjust
	breakpoint address for watchpoints or the catch eventpoints.
	Add new paramter bptype.  Adjust all callers.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.162
diff -u -p -r1.162 breakpoint.c
--- breakpoint.c	28 Feb 2004 16:56:12 -0000	1.162
+++ breakpoint.c	15 Mar 2004 21:12:45 -0000
@@ -99,7 +99,8 @@ static void check_duplicates (struct bre
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
-static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr);
+static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr,
+                                            enum bptype bptype);
 
 static void describe_other_breakpoints (CORE_ADDR, asection *);
 
@@ -3947,13 +3948,25 @@ breakpoint_adjustment_warning (CORE_ADDR
    this function is simply the identity function.  */
 
 static CORE_ADDR
-adjust_breakpoint_address (CORE_ADDR bpaddr)
+adjust_breakpoint_address (CORE_ADDR bpaddr, enum bptype bptype)
 {
   if (!gdbarch_adjust_breakpoint_address_p (current_gdbarch))
     {
       /* Very few targets need any kind of breakpoint adjustment.  */
       return bpaddr;
     }
+  else if (bptype == bp_watchpoint
+           || bptype == bp_hardware_watchpoint
+           || bptype == bp_read_watchpoint
+           || bptype == bp_access_watchpoint
+           || bptype == bp_catch_fork
+           || bptype == bp_catch_vfork
+           || bptype == bp_catch_exec)
+    {
+      /* Watchpoints and the various bp_catch_* eventpoints should not
+         have their addresses modified.  */
+      return bpaddr;
+    }
   else
     {
       CORE_ADDR adjusted_bpaddr;
@@ -4062,7 +4075,8 @@ set_raw_breakpoint (struct symtab_and_li
   memset (b, 0, sizeof (*b));
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
-  b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+  b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                               bptype);
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else
@@ -4622,7 +4636,8 @@ set_longjmp_resume_breakpoint (CORE_ADDR
     if (b->type == bp_longjmp_resume)
     {
       b->loc->requested_address = pc;
-      b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+      b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                                   b->type);
       b->enable_state = bp_enabled;
       b->frame_id = frame_id;
       check_duplicates (b);
@@ -5879,7 +5894,8 @@ watch_command_1 (char *arg, int accessfl
 	  scope_breakpoint->loc->requested_address
 	    = get_frame_pc (prev_frame);
 	  scope_breakpoint->loc->address
-	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address);
+	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
+	                                 scope_breakpoint->type);
 
 	  /* The scope breakpoint is related to the watchpoint.  We
 	     will need to act on them together.  */
@@ -7190,7 +7206,8 @@ breakpoint_re_set_one (void *bint)
 	      b->line_number = sals.sals[i].line;
 	      b->loc->requested_address = sals.sals[i].pc;
 	      b->loc->address
-	        = adjust_breakpoint_address (b->loc->requested_address);
+	        = adjust_breakpoint_address (b->loc->requested_address,
+		                             b->type);
 
 	      /* Used to check for duplicates here, but that can
 	         cause trouble, as it doesn't check for disabled

Re: [RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

On Mon, 15 Mar 2004 14:18:00 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> 	* breakpoint.c (adjust_breakpoint_address): Don't adjust
> 	breakpoint address for watchpoints or the catch eventpoints.
> 	Add new paramter bptype.  Adjust all callers.

Committed.

Kevin

[RFC] breakpoint.c: Don't let adjust_breakpoint_address() adjust watchpoints

[Kevin Buettner]

In the course of running the GDB testsuite on FR-V, I discovered that it's
a bad idea to adjust watchpoint addresses.  If any adjustment is done, the
watchpoint ends up at the wrong address.  The patch below simply disables
adjustment for watchpoints and some eventpoints.

Any comments on this patch?  If not, I'll commit it towards the end of the
week.

Kevin

	* breakpoint.c (adjust_breakpoint_address): Don't adjust
	breakpoint address for watchpoints or the catch eventpoints.
	Add new paramter bptype.  Adjust all callers.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.162
diff -u -p -r1.162 breakpoint.c
--- breakpoint.c	28 Feb 2004 16:56:12 -0000	1.162
+++ breakpoint.c	15 Mar 2004 20:18:45 -0000
@@ -99,7 +99,8 @@ static void check_duplicates (struct bre
 
 static void breakpoint_adjustment_warning (CORE_ADDR, CORE_ADDR, int, int);
 
-static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr);
+static CORE_ADDR adjust_breakpoint_address (CORE_ADDR bpaddr,
+                                            enum bptype bptype);
 
 static void describe_other_breakpoints (CORE_ADDR, asection *);
 
@@ -3947,13 +3948,25 @@ breakpoint_adjustment_warning (CORE_ADDR
    this function is simply the identity function.  */
 
 static CORE_ADDR
-adjust_breakpoint_address (CORE_ADDR bpaddr)
+adjust_breakpoint_address (CORE_ADDR bpaddr, enum bptype bptype)
 {
   if (!gdbarch_adjust_breakpoint_address_p (current_gdbarch))
     {
       /* Very few targets need any kind of breakpoint adjustment.  */
       return bpaddr;
     }
+  else if (bptype == bp_watchpoint
+           || bptype == bp_hardware_watchpoint
+           || bptype == bp_read_watchpoint
+           || bptype == bp_access_watchpoint
+           || bptype == bp_catch_fork
+           || bptype == bp_catch_vfork
+           || bptype == bp_catch_exec)
+    {
+      /* Watchpoints and the various bp_catch_* eventpoints should not
+         have their addresses modified.  */
+      return bpaddr;
+    }
   else
     {
       CORE_ADDR adjusted_bpaddr;
@@ -4062,7 +4075,8 @@ set_raw_breakpoint (struct symtab_and_li
   memset (b, 0, sizeof (*b));
   b->loc = allocate_bp_location (b, bptype);
   b->loc->requested_address = sal.pc;
-  b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+  b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                               bptype);
   if (sal.symtab == NULL)
     b->source_file = NULL;
   else
@@ -4622,7 +4636,8 @@ set_longjmp_resume_breakpoint (CORE_ADDR
     if (b->type == bp_longjmp_resume)
     {
       b->loc->requested_address = pc;
-      b->loc->address = adjust_breakpoint_address (b->loc->requested_address);
+      b->loc->address = adjust_breakpoint_address (b->loc->requested_address,
+                                                   b->type);
       b->enable_state = bp_enabled;
       b->frame_id = frame_id;
       check_duplicates (b);
@@ -5879,7 +5894,8 @@ watch_command_1 (char *arg, int accessfl
 	  scope_breakpoint->loc->requested_address
 	    = get_frame_pc (prev_frame);
 	  scope_breakpoint->loc->address
-	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address);
+	    = adjust_breakpoint_address (scope_breakpoint->loc->requested_address,
+	                                 scope_breakpoint->type);
 
 	  /* The scope breakpoint is related to the watchpoint.  We
 	     will need to act on them together.  */
@@ -5894,11 +5910,6 @@ watch_command_1 (char *arg, int accessfl
    in hardware.  If the watchpoint can not be handled
    in hardware return zero.  */
 
-#if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
-#define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(BYTE_SIZE) \
-    ((BYTE_SIZE) <= (DEPRECATED_REGISTER_SIZE))
-#endif
-
 #if !defined(TARGET_REGION_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_OK_FOR_HW_WATCHPOINT(ADDR,LEN) \
      (TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(LEN))
@@ -7190,7 +7201,8 @@ breakpoint_re_set_one (void *bint)
 	      b->line_number = sals.sals[i].line;
 	      b->loc->requested_address = sals.sals[i].pc;
 	      b->loc->address
-	        = adjust_breakpoint_address (b->loc->requested_address);
+	        = adjust_breakpoint_address (b->loc->requested_address,
+		                             b->type);
 
 	      /* Used to check for duplicates here, but that can
 	         cause trouble, as it doesn't check for disabled