[RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Kevin Buettner]

I've recently revisited a patch that I submitted a long time ago. Here
are the relevant links to the original TARGET_ADJUST_BREAKPOINT_ADDRESS
patch submission and discussion:

     http://sources.redhat.com/ml/gdb-patches/2000-06/msg00257.html
 (+) http://sources.redhat.com/ml/gdb-patches/2000-06/msg00260.html
     http://sources.redhat.com/ml/gdb-patches/2000-06/msg00270.html
 (+) http://sources.redhat.com/ml/gdb-patches/2000-06/msg00274.html
 (*) http://sources.redhat.com/ml/gdb-patches/2000-09/msg00156.html
     http://sources.redhat.com/ml/gdb-patches/2000-09/msg00158.html

The link marked with (*) is Andrew's approval so long as Eli's
concerns marked with (+) are addressed.

The patch below should be non-controversial.  It merely adds the
TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
possibly controversial bit) will come in patch #3.  Documentation will
be in the next patch, #2.  Finally, target specific code which
requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.

Kevin

	* arch-utils.h (default_target_adjust_breakpoint_address): New
	function declaration.
	* arch-utils.c (default_target_adjust_breakpoint_address): New
	function.
	* gdbarch.sh (TARGET_ADJUST_BREAKPOINT_ADDRESS): New method.
	* gdbarch.h, gdbarch.c: Regenerate.

Index: arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.97
diff -u -p -r1.97 arch-utils.c
--- arch-utils.c	30 Sep 2003 13:29:43 -0000	1.97
+++ arch-utils.c	3 Oct 2003 23:38:23 -0000
@@ -82,6 +82,21 @@ legacy_register_sim_regno (int regnum)
     return LEGACY_SIM_REGNO_IGNORE;
 }
 
+/* Some architectures have constraints about where a breakpoint may be
+   placed.  TARGET_ADJUST_BREAKPOINT_ADDRESS is given an address that is
+   adjusted (and returned) to conform to the constraints imposed by
+   the architecture.
+   
+   Targets which have no architectural constraints on breakpoint placement
+   should use default_target_adjust_breakpoint_address which merely returns
+   the breakpoint address unadjusted */
+
+CORE_ADDR
+default_target_adjust_breakpoint_address (CORE_ADDR bpaddr)
+{
+  return bpaddr;
+}
+
 int
 generic_frameless_function_invocation_not (struct frame_info *fi)
 {
Index: arch-utils.h
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.h,v
retrieving revision 1.57
diff -u -p -r1.57 arch-utils.h
--- arch-utils.h	27 Sep 2003 15:51:02 -0000	1.57
+++ arch-utils.h	3 Oct 2003 23:38:23 -0000
@@ -47,6 +47,11 @@ extern gdbarch_store_return_value_ftype 
    address passed as an invisible first argument to the function.  */
 extern gdbarch_use_struct_convention_ftype always_use_struct_convention;
 
+/* Identity function for targets which don't have architectural constraints
+   on placement of breakpoints. */
+extern gdbarch_target_adjust_breakpoint_address_ftype
+  default_target_adjust_breakpoint_address;
+
 /* Frameless functions not identifable. */
 extern gdbarch_frameless_function_invocation_ftype generic_frameless_function_invocation_not;
 
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.275
diff -u -p -r1.275 gdbarch.sh
--- gdbarch.sh	2 Oct 2003 20:28:29 -0000	1.275
+++ gdbarch.sh	3 Oct 2003 23:38:27 -0000
@@ -614,6 +614,7 @@ f:2:SKIP_PROLOGUE:CORE_ADDR:skip_prologu
 f:2:PROLOGUE_FRAMELESS_P:int:prologue_frameless_p:CORE_ADDR ip:ip::0:generic_prologue_frameless_p::0
 f:2:INNER_THAN:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs::0:0
 f::BREAKPOINT_FROM_PC:const unsigned char *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr:::0:
+f:2:TARGET_ADJUST_BREAKPOINT_ADDRESS:CORE_ADDR:target_adjust_breakpoint_address:CORE_ADDR bpaddr:bpaddr:::default_target_adjust_breakpoint_address::0
 f:2:MEMORY_INSERT_BREAKPOINT:int:memory_insert_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_insert_breakpoint::0
 f:2:MEMORY_REMOVE_BREAKPOINT:int:memory_remove_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_remove_breakpoint::0
 v:2:DECR_PC_AFTER_BREAK:CORE_ADDR:decr_pc_after_break::::0:-1

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Mark Kettenis]

   Date: Fri, 3 Oct 2003 17:28:13 -0700
   From: Kevin Buettner <kevinb@redhat.com>

   The patch below should be non-controversial.  It merely adds the
   TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
   possibly controversial bit) will come in patch #3.  Documentation will
   be in the next patch, #2.  Finally, target specific code which
   requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.

Do we really need the "TARGET" in the name of the new method?  It made
me think that this was something that was going to be added to the
target-vector instead of the architecture vector.

Mark

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Andrew Cagney]

>    Date: Fri, 3 Oct 2003 17:28:13 -0700
>    From: Kevin Buettner <kevinb@redhat.com>
> 
>    The patch below should be non-controversial.  It merely adds the
>    TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
>    possibly controversial bit) will come in patch #3.  Documentation will
>    be in the next patch, #2.  Finally, target specific code which
>    requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.
> 
> Do we really need the "TARGET" in the name of the new method?  It made
> me think that this was something that was going to be added to the
> target-vector instead of the architecture vector.

Good point (making note that the term 'target architecture' really 
doesn't make sense).

Andrew

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Kevin Buettner]

On Oct 6,  2:49pm, Mark Kettenis wrote:

>    The patch below should be non-controversial.  It merely adds the
>    TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
>    possibly controversial bit) will come in patch #3.  Documentation will
>    be in the next patch, #2.  Finally, target specific code which
>    requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.
> 
> Do we really need the "TARGET" in the name of the new method?  It made
> me think that this was something that was going to be added to the
> target-vector instead of the architecture vector.

How about just "ADJUST_BREAKPOINT_ADDRESS" ?

Kevin

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Mark Kettenis]

   Date: Mon, 6 Oct 2003 08:36:58 -0700
   From: Kevin Buettner <kevinb@redhat.com>

   On Oct 6,  2:49pm, Mark Kettenis wrote:

   >    The patch below should be non-controversial.  It merely adds the
   >    TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
   >    possibly controversial bit) will come in patch #3.  Documentation will
   >    be in the next patch, #2.  Finally, target specific code which
   >    requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.
   > 
   > Do we really need the "TARGET" in the name of the new method?  It made
   > me think that this was something that was going to be added to the
   > target-vector instead of the architecture vector.

   How about just "ADJUST_BREAKPOINT_ADDRESS" ?

Sounds fine to me :-).  Thanks,

Mark

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Andrew Cagney]

>    Date: Mon, 6 Oct 2003 08:36:58 -0700
>    From: Kevin Buettner <kevinb@redhat.com>
> 
>    On Oct 6,  2:49pm, Mark Kettenis wrote:
> 
>    >    The patch below should be non-controversial.  It merely adds the
>    >    TARGET_ADJUST_BREAKPOINT_ADDRESS method.  Code which uses it (the
>    >    possibly controversial bit) will come in patch #3.  Documentation will
>    >    be in the next patch, #2.  Finally, target specific code which
>    >    requires TARGET_ADJUST_BREAKPOINT_ADDRESS will be posted in patch #4.
>    > 
>    > Do we really need the "TARGET" in the name of the new method?  It made
>    > me think that this was something that was going to be added to the
>    > target-vector instead of the architecture vector.
> 
>    How about just "ADJUST_BREAKPOINT_ADDRESS" ?
> 
> Sounds fine to me :-).  Thanks,

Then ADJUST_BREAKPOINT_ADDRESS is the name.

[snip]
> To review the criticism of the previous TARGET_ADJUST_BREAKPOINT
> patch (from three years ago!), see:

Since then, the architecture vector has gained "m" and "M" - i.e., 
strictly multi-arch - and the new method will need to be one of those. 
Looking at:

+/* Call TARGET_ADJUST_BREAKPOINT_ADDRESS and print warning if an address
+   adjustment was made.  Returns the adjusted address. */
+
+static CORE_ADDR
+adjust_breakpoint_address (CORE_ADDR bpaddr)
+{
+  CORE_ADDR adjusted_bpaddr;
+
+  adjusted_bpaddr = TARGET_ADJUST_BREAKPOINT_ADDRESS (bpaddr);
+
+  if (adjusted_bpaddr != bpaddr)
+    breakpoint_adjustment_warning (bpaddr, adjusted_bpaddr, 0, 0);
+
+  return adjusted_bpaddr;
+}
+

I've a strong preference for "M" - method with predicate - since, for 
everyone but an frv developer, this code is irrelevant.

Having a predicate would let adjust_breakpoint_address be written in a 
style that reflects this atypical code path:

	if (gdbarch_adjust_..._p())
	  adjusted = ...
	  if (changed)
	    warning ...

and the average (non-frv) developer wouldn't be distracted by the change.

Other than that, the architect method is ok.

Andrew

PS:

I wonder if the other parts of your change slam straight into DanielJ's 
two teer breakpoint effect (assuming I remember his long ago intent) vis:
- the high-level user specified breakpoint (requested_address?)
- the low-level corresponding list of real breakpoints (address?)

I also wonder if PPC64 GDB could use this to do the descriptor -> 
function address transformation that I described in:
http://sources.redhat.com/ml/gdb-patches/2003-09/msg00415.html

Andrew

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Kevin Buettner]

On Oct 8,  2:26pm, Andrew Cagney wrote:

> I also wonder if PPC64 GDB could use this to do the descriptor -> 
> function address transformation that I described in:
> http://sources.redhat.com/ml/gdb-patches/2003-09/msg00415.html

I suppose it could, although that's definitely not the (well, my)
intended purpose of ADJUST_BREAKPOINT_ADDRESS.  If we do wind up using
it for this purpose, we'll want to disable the warning messages.  It
makes no sense to have a breakpoint on the descriptor, so when used in
this way, it'd definitely be the case that gdb is just doing the
"right thing".  It'd be easy enough to add an argument to the method
which passes indicating the kind of adjustment.  Off hand, I think a
tri-state value makes sense:


    1) the adjustment can potentially alter expected behavior making
       user warnings manditory.  E.g, FR-V architecture constraints.

    2) benign, the breakpoint's location has been moved slightly,
       but there should be no change in expected behavior.  Perhaps
       an informational message could be displayed for this state.
       E.g.  - maybe - that old v850 problem that Gary Thomas once
       told us about in which he had to sometimes place breakpoints
       that were larger than the smallest instruction.

    3) the adjustment was necessary because to place a breakpoint on
       the original address is wrong.  E.g, function descriptors -
       it makes no sense to place a breakpoint on the function descriptor,
       but it does make sense to place a breakpoint on the code address
       that the descriptor points to.

Kevin

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Andrew Cagney]

> On Oct 8,  2:26pm, Andrew Cagney wrote:
> 
> 
>> I also wonder if PPC64 GDB could use this to do the descriptor -> 
>> function address transformation that I described in:
>> http://sources.redhat.com/ml/gdb-patches/2003-09/msg00415.html
> 
> 
> I suppose it could, although that's definitely not the (well, my)
> intended purpose of ADJUST_BREAKPOINT_ADDRESS.  If we do wind up using
> it for this purpose, we'll want to disable the warning messages.  It
> makes no sense to have a breakpoint on the descriptor, so when used in
> this way, it'd definitely be the case that gdb is just doing the
> "right thing".  It'd be easy enough to add an argument to the method
> which passes indicating the kind of adjustment.  Off hand, I think a
> tri-state value makes sense:

right.

>     1) the adjustment can potentially alter expected behavior making
>        user warnings manditory.  E.g, FR-V architecture constraints.
> 
>     2) benign, the breakpoint's location has been moved slightly,
>        but there should be no change in expected behavior.  Perhaps
>        an informational message could be displayed for this state.
>        E.g.  - maybe - that old v850 problem that Gary Thomas once
>        told us about in which he had to sometimes place breakpoints
>        that were larger than the smallest instruction.
> 
>     3) the adjustment was necessary because to place a breakpoint on
>        the original address is wrong.  E.g, function descriptors -
>        it makes no sense to place a breakpoint on the function descriptor,
>        but it does make sense to place a breakpoint on the code address
>        that the descriptor points to.

or:

	if (the architecture can adjust things)
	  adjust
	  if (the architecture thinks the adjustment is weird)
	    warning

A follow-on change can refine it.

Andrew

Re: [RFC] TARGET_ADJUST_BREAKPOINT_ADDRESS - patch 1 of 4

[Kevin Buettner]

On Oct 3,  5:28pm, Kevin Buettner wrote:

> 	* arch-utils.h (default_target_adjust_breakpoint_address): New
> 	function declaration.
> 	* arch-utils.c (default_target_adjust_breakpoint_address): New
> 	function.
> 	* gdbarch.sh (TARGET_ADJUST_BREAKPOINT_ADDRESS): New method.
> 	* gdbarch.h, gdbarch.c: Regenerate.

I reworked this patch per Andrew's and Mark's suggestions.  Using an
"M" (method with predicate) eliminated the need for the
arch-utils.[hc] changes.

Here's what I've checked in:

 	* gdbarch.sh (ADJUST_BREAKPOINT_ADDRESS): New method.
 	* gdbarch.h, gdbarch.c: Regenerate.

Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.277
diff -u -p -r1.277 gdbarch.sh
--- gdbarch.sh	11 Oct 2003 12:52:29 -0000	1.277
+++ gdbarch.sh	13 Oct 2003 23:33:27 -0000
@@ -614,6 +614,7 @@ f:2:SKIP_PROLOGUE:CORE_ADDR:skip_prologu
 f:2:PROLOGUE_FRAMELESS_P:int:prologue_frameless_p:CORE_ADDR ip:ip::0:generic_prologue_frameless_p::0
 f:2:INNER_THAN:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs::0:0
 f::BREAKPOINT_FROM_PC:const unsigned char *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr:::0:
+M:2:ADJUST_BREAKPOINT_ADDRESS:CORE_ADDR:adjust_breakpoint_address:CORE_ADDR bpaddr:bpaddr
 f:2:MEMORY_INSERT_BREAKPOINT:int:memory_insert_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_insert_breakpoint::0
 f:2:MEMORY_REMOVE_BREAKPOINT:int:memory_remove_breakpoint:CORE_ADDR addr, char *contents_cache:addr, contents_cache::0:default_memory_remove_breakpoint::0
 v:2:DECR_PC_AFTER_BREAK:CORE_ADDR:decr_pc_after_break::::0:-1