[PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

Hi.

watchpoint_update and watchpoint_cond avoid checking for watchpoints
when we are located at a function epilogue in the current frame.  This
is done in order to avoid using corrupted local registers and unwinding
a corrupted/destroyed stack.

The code determining whether we are in a function epilogue is provided
by the backends via the gdbarch_in_function_epilogue_p hook.  The patch
below adds such a hook for sparc.

Note that despite sparc_in_function_epilogue_p must work on both sparc32
and sparc64 the patch only installs the hook on sparc64 targets.  This
is because I can't test it in sparc32.

This patch makes all the tests on watch-cond.exp to pass on
sparc64-unknown-linux-gnu.

2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
	(X_RETTURN): New macro.
	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.

	* sparc64-tdep.c (sparc64_init_abi): Hook
	sparc_in_function_epilogue_p.


Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.233
diff -u -r1.233 sparc-tdep.c
--- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
+++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -88,6 +88,8 @@
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
 #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
 #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
+/* Macros to identify some instructions.  */
+#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -421,6 +434,29 @@
   regcache_raw_write (regcache, regnum + 1, buf + 4);
 }
 \f
+/* Return true if we are in a function's epilogue, i.e. after an
+   instruction that destroyed a function's stack frame.  */
+
+int
+sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  /* This function must return true if we are one instruction after an
+     instruction that destroyed the stack frame of the current
+     function.  The SPARC instructions used to restore the callers
+     stack frame are RESTORE and RETURN/RETT.
+
+     Of these RETURN/RETT is a branch instruction and thus we return
+     true if we are in its delay slot.
+
+     RESTORE is almost always found in the delay slot of a branch
+     instruction that transfer control to the caller, such as JMPL.
+     Thus the next instruction is in the caller frame and we don't
+     need to do anything about it.  */
+
+  unsigned int insn = sparc_fetch_instruction (pc - 4);  
+  return X_RETTURN (insn);
+}
+\f
 
 static CORE_ADDR
 sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
Index: sparc-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
retrieving revision 1.33
diff -u -r1.33 sparc-tdep.h
--- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
+++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
@@ -193,6 +193,9 @@
 extern struct sparc_frame_cache *
   sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
 
+extern int
+sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
+
 \f
 
 extern int sparc_software_single_step (struct frame_info *frame);
Index: sparc64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
retrieving revision 1.62
diff -u -r1.62 sparc64-tdep.c
--- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
+++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -1197,6 +1198,9 @@
 
   set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
+
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
   /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Sergio Durigan Junior]

On Wednesday, October 16 2013, Jose E. Marchesi wrote:

> Hi.
>
[...]
> 2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
>
> 	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
> 	(X_RETTURN): New macro.
> 	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
>
> 	* sparc64-tdep.c (sparc64_init_abi): Hook
> 	sparc_in_function_epilogue_p.
>
>
[...]
> Index: sparc-tdep.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
> retrieving revision 1.33
> diff -u -r1.33 sparc-tdep.h
> --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
> +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
> @@ -193,6 +193,9 @@
>  extern struct sparc_frame_cache *
>    sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
>  
> +extern int
> +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);

Not really a review, but the function name should not start on column 0
for prototypes.

Thanks,

-- 
Sergio

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

    > Index: sparc-tdep.h
    > ===================================================================
    > RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
    > retrieving revision 1.33
    > diff -u -r1.33 sparc-tdep.h
    > --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
    > +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
    > @@ -193,6 +193,9 @@
    >  extern struct sparc_frame_cache *
    >    sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
    >  
    > +extern int
    > +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
    
    Not really a review, but the function name should not start on column 0
    for prototypes.

Thanks for noticing out.
Amended patch below.

2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
	(X_RETTURN): New macro.
	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.

	* sparc64-tdep.c (sparc64_init_abi): Hook
	sparc_in_function_epilogue_p.


Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.233
diff -u -r1.233 sparc-tdep.c
--- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
+++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -88,6 +88,8 @@
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
 #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
 #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
+/* Macros to identify some instructions.  */
+#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -421,6 +434,29 @@
   regcache_raw_write (regcache, regnum + 1, buf + 4);
 }
 \f
+/* Return true if we are in a function's epilogue, i.e. after an
+   instruction that destroyed a function's stack frame.  */
+
+int
+sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  /* This function must return true if we are one instruction after an
+     instruction that destroyed the stack frame of the current
+     function.  The SPARC instructions used to restore the callers
+     stack frame are RESTORE and RETURN/RETT.
+
+     Of these RETURN/RETT is a branch instruction and thus we return
+     true if we are in its delay slot.
+
+     RESTORE is almost always found in the delay slot of a branch
+     instruction that transfer control to the caller, such as JMPL.
+     Thus the next instruction is in the caller frame and we don't
+     need to do anything about it.  */
+
+  unsigned int insn = sparc_fetch_instruction (pc - 4);  
+  return X_RETTURN (insn);
+}
+\f
 
 static CORE_ADDR
 sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
Index: sparc-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
retrieving revision 1.33
diff -u -r1.33 sparc-tdep.h
--- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
+++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
@@ -193,6 +193,9 @@
 extern struct sparc_frame_cache *
   sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
 
+extern int
+  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
+
 \f
 
 extern int sparc_software_single_step (struct frame_info *frame);
Index: sparc64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
retrieving revision 1.62
diff -u -r1.62 sparc64-tdep.c
--- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
+++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -1197,6 +1198,9 @@
 
   set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
+
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
   /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

ping

        > Index: sparc-tdep.h
        > ===================================================================
        > RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
        > retrieving revision 1.33
        > diff -u -r1.33 sparc-tdep.h
        > --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
        > +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
        > @@ -193,6 +193,9 @@
        >  extern struct sparc_frame_cache *
        >    sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
        >  
        > +extern int
        > +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
        
        Not really a review, but the function name should not start on column 0
        for prototypes.
    
    Thanks for noticing out.
    Amended patch below.
    
    2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
    
    	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
    	(X_RETTURN): New macro.
    	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
    
    	* sparc64-tdep.c (sparc64_init_abi): Hook
    	sparc_in_function_epilogue_p.
    
    
    Index: sparc-tdep.c
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
    retrieving revision 1.233
    diff -u -r1.233 sparc-tdep.c
    --- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
    +++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
    @@ -88,6 +88,8 @@
     #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
     #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
     #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
    +/* Macros to identify some instructions.  */
    +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
     
     /* Fetch the instruction at PC.  Instructions are always big-endian
        even if the processor operates in little-endian mode.  */
    @@ -421,6 +434,29 @@
       regcache_raw_write (regcache, regnum + 1, buf + 4);
     }
     \f
    +/* Return true if we are in a function's epilogue, i.e. after an
    +   instruction that destroyed a function's stack frame.  */
    +
    +int
    +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
    +{
    +  /* This function must return true if we are one instruction after an
    +     instruction that destroyed the stack frame of the current
    +     function.  The SPARC instructions used to restore the callers
    +     stack frame are RESTORE and RETURN/RETT.
    +
    +     Of these RETURN/RETT is a branch instruction and thus we return
    +     true if we are in its delay slot.
    +
    +     RESTORE is almost always found in the delay slot of a branch
    +     instruction that transfer control to the caller, such as JMPL.
    +     Thus the next instruction is in the caller frame and we don't
    +     need to do anything about it.  */
    +
    +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
    +  return X_RETTURN (insn);
    +}
    +\f
     
     static CORE_ADDR
     sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
    Index: sparc-tdep.h
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
    retrieving revision 1.33
    diff -u -r1.33 sparc-tdep.h
    --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
    +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
    @@ -193,6 +193,9 @@
     extern struct sparc_frame_cache *
       sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
     
    +extern int
    +  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
    +
     \f
     
     extern int sparc_software_single_step (struct frame_info *frame);
    Index: sparc64-tdep.c
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
    retrieving revision 1.62
    diff -u -r1.62 sparc64-tdep.c
    --- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
    +++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
    @@ -1197,6 +1198,9 @@
     
       set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
     
    +  /* Detect whether PC is in function epilogue.  */
    +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
    +
       /* Hook in the DWARF CFI frame unwinder.  */
       dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
       /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Pedro Alves]

On 10/16/2013 03:18 PM, Jose E. Marchesi wrote:
> Note that despite sparc_in_function_epilogue_p must work on both sparc32
> and sparc64 the patch only installs the hook on sparc64 targets.  This
> is because I can't test it in sparc32.

Can't sparc64 run sparc32 binaries?  Something like -m32 ?

-- 
Pedro Alves

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

    On 10/16/2013 03:18 PM, Jose E. Marchesi wrote:
    > Note that despite sparc_in_function_epilogue_p must work on both sparc32
    > and sparc64 the patch only installs the hook on sparc64 targets.  This
    > is because I can't test it in sparc32.
    
    Can't sparc64 run sparc32 binaries?  Something like -m32 ?

Yes it can.  But right now what I have is a 64bit-only userspace.  I
will hook gdbarch_in_function_epilogue_p on the sparc32 target as soon
as I can test it properly.

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Pedro Alves]

On 12/04/2013 10:08 AM, Jose E. Marchesi wrote:
> 
>     On 10/16/2013 03:18 PM, Jose E. Marchesi wrote:
>     > Note that despite sparc_in_function_epilogue_p must work on both sparc32
>     > and sparc64 the patch only installs the hook on sparc64 targets.  This
>     > is because I can't test it in sparc32.
>     
>     Can't sparc64 run sparc32 binaries?  Something like -m32 ?
> 
> Yes it can.  But right now what I have is a 64bit-only userspace.  I
> will hook gdbarch_in_function_epilogue_p on the sparc32 target as soon
> as I can test it properly.

Ah, OK.

>     +     RESTORE is almost always found in the delay slot of a branch
>     +     instruction that transfer control to the caller, such as JMPL.

"transfers".

I don't know enough sparc64 to be able to say anything intelligent
about the substance of the patch itself.

-- 
Pedro Alves

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

    >     +     RESTORE is almost always found in the delay slot of a branch
    >     +     instruction that transfer control to the caller, such as JMPL.
    
    "transfers".

Typo amended below.

2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
	(X_RETTURN): New macro.
	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.

	* sparc64-tdep.c (sparc64_init_abi): Hook
	sparc_in_function_epilogue_p.


Index: sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.233
diff -u -r1.233 sparc-tdep.c
--- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
+++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -88,6 +88,8 @@
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
 #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
 #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
+/* Macros to identify some instructions.  */
+#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -421,6 +434,29 @@
   regcache_raw_write (regcache, regnum + 1, buf + 4);
 }
 \f
+/* Return true if we are in a function's epilogue, i.e. after an
+   instruction that destroyed a function's stack frame.  */
+
+int
+sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  /* This function must return true if we are one instruction after an
+     instruction that destroyed the stack frame of the current
+     function.  The SPARC instructions used to restore the callers
+     stack frame are RESTORE and RETURN/RETT.
+
+     Of these RETURN/RETT is a branch instruction and thus we return
+     true if we are in its delay slot.
+
+     RESTORE is almost always found in the delay slot of a branch
+     instruction that transfers control to the caller, such as JMPL.
+     Thus the next instruction is in the caller frame and we don't
+     need to do anything about it.  */
+
+  unsigned int insn = sparc_fetch_instruction (pc - 4);  
+  return X_RETTURN (insn);
+}
+\f
 
 static CORE_ADDR
 sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
Index: sparc-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
retrieving revision 1.33
diff -u -r1.33 sparc-tdep.h
--- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
+++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
@@ -193,6 +193,9 @@
 extern struct sparc_frame_cache *
   sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
 
+extern int
+  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
+
 \f
 
 extern int sparc_software_single_step (struct frame_info *frame);
Index: sparc64-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
retrieving revision 1.62
diff -u -r1.62 sparc64-tdep.c
--- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
+++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
@@ -1197,6 +1198,9 @@
 
   set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
 
+  /* Detect whether PC is in function epilogue.  */
+  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
+
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
   /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Joel Brobecker]

> >     On 10/16/2013 03:18 PM, Jose E. Marchesi wrote:
> >     > Note that despite sparc_in_function_epilogue_p must work on both sparc32
> >     > and sparc64 the patch only installs the hook on sparc64 targets.  This
> >     > is because I can't test it in sparc32.
> >     
> >     Can't sparc64 run sparc32 binaries?  Something like -m32 ?
> > 
> > Yes it can.  But right now what I have is a 64bit-only userspace.  I
> > will hook gdbarch_in_function_epilogue_p on the sparc32 target as soon
> > as I can test it properly.
> 
> Ah, OK.
> 
> >     +     RESTORE is almost always found in the delay slot of a branch
> >     +     instruction that transfer control to the caller, such as JMPL.

I have access to sparc32 systems running Solaris, except I am under
strict orders to not run the official testsuite on it. But if there is
something else I can do, including running AdaCore's testsuite, or
doing a specific check by hand, etc, do let me know!

-- 
Joel

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

ping


        >     +     RESTORE is almost always found in the delay slot of a branch
        >     +     instruction that transfer control to the caller, such as JMPL.
        
        "transfers".
    
    Typo amended below.
    
    2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
    
    	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
    	(X_RETTURN): New macro.
    	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
    
    	* sparc64-tdep.c (sparc64_init_abi): Hook
    	sparc_in_function_epilogue_p.
    
    
    Index: sparc-tdep.c
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
    retrieving revision 1.233
    diff -u -r1.233 sparc-tdep.c
    --- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
    +++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
    @@ -88,6 +88,8 @@
     #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
     #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
     #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
    +/* Macros to identify some instructions.  */
    +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
     
     /* Fetch the instruction at PC.  Instructions are always big-endian
        even if the processor operates in little-endian mode.  */
    @@ -421,6 +434,29 @@
       regcache_raw_write (regcache, regnum + 1, buf + 4);
     }
     \f
    +/* Return true if we are in a function's epilogue, i.e. after an
    +   instruction that destroyed a function's stack frame.  */
    +
    +int
    +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
    +{
    +  /* This function must return true if we are one instruction after an
    +     instruction that destroyed the stack frame of the current
    +     function.  The SPARC instructions used to restore the callers
    +     stack frame are RESTORE and RETURN/RETT.
    +
    +     Of these RETURN/RETT is a branch instruction and thus we return
    +     true if we are in its delay slot.
    +
    +     RESTORE is almost always found in the delay slot of a branch
    +     instruction that transfers control to the caller, such as JMPL.
    +     Thus the next instruction is in the caller frame and we don't
    +     need to do anything about it.  */
    +
    +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
    +  return X_RETTURN (insn);
    +}
    +\f
     
     static CORE_ADDR
     sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
    Index: sparc-tdep.h
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
    retrieving revision 1.33
    diff -u -r1.33 sparc-tdep.h
    --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
    +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
    @@ -193,6 +193,9 @@
     extern struct sparc_frame_cache *
       sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
     
    +extern int
    +  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
    +
     \f
     
     extern int sparc_software_single_step (struct frame_info *frame);
    Index: sparc64-tdep.c
    ===================================================================
    RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
    retrieving revision 1.62
    diff -u -r1.62 sparc64-tdep.c
    --- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
    +++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
    @@ -1197,6 +1198,9 @@
     
       set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
     
    +  /* Detect whether PC is in function epilogue.  */
    +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
    +
       /* Hook in the DWARF CFI frame unwinder.  */
       dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
       /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

ping
    
    
            >     +     RESTORE is almost always found in the delay slot of a branch
            >     +     instruction that transfer control to the caller, such as JMPL.
            
            "transfers".
        
        Typo amended below.
        
        2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
        
        	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
        	(X_RETTURN): New macro.
        	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
        
        	* sparc64-tdep.c (sparc64_init_abi): Hook
        	sparc_in_function_epilogue_p.
        
        
        Index: sparc-tdep.c
        ===================================================================
        RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
        retrieving revision 1.233
        diff -u -r1.233 sparc-tdep.c
        --- sparc-tdep.c	24 Jun 2013 22:18:32 -0000	1.233
        +++ sparc-tdep.c	16 Oct 2013 14:00:49 -0000
        @@ -88,6 +88,8 @@
         #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
         #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
         #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
        +/* Macros to identify some instructions.  */
        +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
         
         /* Fetch the instruction at PC.  Instructions are always big-endian
            even if the processor operates in little-endian mode.  */
        @@ -421,6 +434,29 @@
           regcache_raw_write (regcache, regnum + 1, buf + 4);
         }
         \f
        +/* Return true if we are in a function's epilogue, i.e. after an
        +   instruction that destroyed a function's stack frame.  */
        +
        +int
        +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
        +{
        +  /* This function must return true if we are one instruction after an
        +     instruction that destroyed the stack frame of the current
        +     function.  The SPARC instructions used to restore the callers
        +     stack frame are RESTORE and RETURN/RETT.
        +
        +     Of these RETURN/RETT is a branch instruction and thus we return
        +     true if we are in its delay slot.
        +
        +     RESTORE is almost always found in the delay slot of a branch
        +     instruction that transfers control to the caller, such as JMPL.
        +     Thus the next instruction is in the caller frame and we don't
        +     need to do anything about it.  */
        +
        +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
        +  return X_RETTURN (insn);
        +}
        +\f
         
         static CORE_ADDR
         sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
        Index: sparc-tdep.h
        ===================================================================
        RCS file: /cvs/src/src/gdb/sparc-tdep.h,v
        retrieving revision 1.33
        diff -u -r1.33 sparc-tdep.h
        --- sparc-tdep.h	1 Jan 2013 06:32:51 -0000	1.33
        +++ sparc-tdep.h	16 Oct 2013 14:00:49 -0000
        @@ -193,6 +193,9 @@
         extern struct sparc_frame_cache *
           sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
         
        +extern int
        +  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
        +
         \f
         
         extern int sparc_software_single_step (struct frame_info *frame);
        Index: sparc64-tdep.c
        ===================================================================
        RCS file: /cvs/src/src/gdb/sparc64-tdep.c,v
        retrieving revision 1.62
        diff -u -r1.62 sparc64-tdep.c
        --- sparc64-tdep.c	1 Jan 2013 06:32:51 -0000	1.62
        +++ sparc64-tdep.c	16 Oct 2013 14:00:49 -0000
        @@ -1197,6 +1198,9 @@
         
           set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
         
        +  /* Detect whether PC is in function epilogue.  */
        +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
        +
           /* Hook in the DWARF CFI frame unwinder.  */
           dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
           /* FIXME: kettenis/20050423: Don't enable the unwinder until the

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Joel Brobecker]

Jose,

(a small request to avoid the extra-large indentation when pinging -
thank you!)

>         2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
>         
>         	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
>         	(X_RETTURN): New macro.
>         	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
>         
>         	* sparc64-tdep.c (sparc64_init_abi): Hook
>         	sparc_in_function_epilogue_p.

Regarding testing this function on sparc32:

Can you tell us which testcase in our testsuite this patch fixes?
Although I am allowed to run the testsuite, I can still run individual
testcases by hand (if not too complex, of course). Otherwise, would
you have a small reproducer I could use to test on sparc32?

Comments about the patch below.

> +/* Macros to identify some instructions.  */
> +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))

Can your comment say a little more precisely what instruction it
identifies? I think the parens around the equality operators are
superfluous and should be removed.

> +/* Return true if we are in a function's epilogue, i.e. after an
> +   instruction that destroyed a function's stack frame.  */
> +
> +int
> +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{

The general principle for function meant to be used as gdbarch callbacks
is just to say:

/* Implement "in_function_epilogue_p".  */

That way, if we ever change the function's prototype, we don't have
to update the documentation everywhere.  This callback should only
be documented in gdbarch.sh (and repeated in gdbarch.h, which is
generated from gdbarch.sh).

In your case, since there are 32bit and 64bit versions, you can add
something like, for instance:

   This implementation works on both sparc32 and sparc64.

> +  /* This function must return true if we are one instruction after an
> +     instruction that destroyed the stack frame of the current
> +     function.  The SPARC instructions used to restore the callers
> +     stack frame are RESTORE and RETURN/RETT.
> +
> +     Of these RETURN/RETT is a branch instruction and thus we return
> +     true if we are in its delay slot.
> +
> +     RESTORE is almost always found in the delay slot of a branch
> +     instruction that transfers control to the caller, such as JMPL.
> +     Thus the next instruction is in the caller frame and we don't
> +     need to do anything about it.  */
> +
> +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
> +  return X_RETTURN (insn);

Small quirk of the GDB Coding Style: We require an empty line between
local variable declarations and the statements after. Also, I notice
there are trailing spaces.

>    set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
>  
> +  /* Detect whether PC is in function epilogue.  */
> +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
> +

I would normally not comment on this, but since I've made other
comments, I'll ask that the comment be revmoved, and that you
avoid the empty line between the call to set_gdbarch_skip_prologue
just above and the call to set_gdbarch_in_function_epilogue_p that
you are adding.

I am asking because the comment in this case is only repeating
the obvious without explaining why it's necessary. So it feels
superfluous and better without.

Thank you,
-- 
Joel

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

Hi Joel.  Thanks for the review!

    >         2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
    >         
    >         	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
    >         	(X_RETTURN): New macro.
    >         	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
    >         
    >         	* sparc64-tdep.c (sparc64_init_abi): Hook
    >         	sparc_in_function_epilogue_p.
    
    Regarding testing this function on sparc32:
    
    Can you tell us which testcase in our testsuite this patch fixes?
    Although I am allowed to run the testsuite, I can still run individual
    testcases by hand (if not too complex, of course). Otherwise, would
    you have a small reproducer I could use to test on sparc32?

As mentioned in the original submission, the failing test was
gdb.base/watch-cond.exp.
    
    Comments about the patch below.
    
    > +/* Macros to identify some instructions.  */
    > +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
    
    Can your comment say a little more precisely what instruction it
    identifies? I think the parens around the equality operators are
    superfluous and should be removed.

Fixed in the amended patch below.
   
    > +/* Return true if we are in a function's epilogue, i.e. after an
    > +   instruction that destroyed a function's stack frame.  */
    > +
    > +int
    > +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
    > +{
    
    The general principle for function meant to be used as gdbarch callbacks
    is just to say:
    
    /* Implement "in_function_epilogue_p".  */
    
    That way, if we ever change the function's prototype, we don't have
    to update the documentation everywhere.  This callback should only
    be documented in gdbarch.sh (and repeated in gdbarch.h, which is
    generated from gdbarch.sh).

Fixed in the amended patch below.
    
    In your case, since there are 32bit and 64bit versions, you can add
    something like, for instance:
    
       This implementation works on both sparc32 and sparc64.

That fact is implicitly true for all functions defined in sparc-tdep.c
with prefix sparc_.
    
    > +  /* This function must return true if we are one instruction after an
    > +     instruction that destroyed the stack frame of the current
    > +     function.  The SPARC instructions used to restore the callers
    > +     stack frame are RESTORE and RETURN/RETT.
    > +
    > +     Of these RETURN/RETT is a branch instruction and thus we return
    > +     true if we are in its delay slot.
    > +
    > +     RESTORE is almost always found in the delay slot of a branch
    > +     instruction that transfers control to the caller, such as JMPL.
    > +     Thus the next instruction is in the caller frame and we don't
    > +     need to do anything about it.  */
    > +
    > +  unsigned int insn = sparc_fetch_instruction (pc - 4);  
    > +  return X_RETTURN (insn);
    
    Small quirk of the GDB Coding Style: We require an empty line between
    local variable declarations and the statements after. Also, I notice
    there are trailing spaces.

Fixed in the amended patch below.
    
    >    set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
    >  
    > +  /* Detect whether PC is in function epilogue.  */
    > +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
    > +
    
    I would normally not comment on this, but since I've made other
    comments, I'll ask that the comment be revmoved, and that you
    avoid the empty line between the call to set_gdbarch_skip_prologue
    just above and the call to set_gdbarch_in_function_epilogue_p that
    you are adding.

Fixed in the amended patch below.    
    
2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>

	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
	(X_RETTURN): New macro.
	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.

	* sparc64-tdep.c (sparc64_init_abi): Hook
	sparc_in_function_epilogue_p.


diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
index 38b345b..311a156 100644
--- a/gdb/sparc-tdep.c
+++ b/gdb/sparc-tdep.c
@@ -88,6 +88,9 @@ struct regset;
 #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
 #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
 #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
+/* Macros to identify some instructions.  */
+/* RETURN (RETT in V8) */
+#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
 
 /* Fetch the instruction at PC.  Instructions are always big-endian
    even if the processor operates in little-endian mode.  */
@@ -452,6 +455,29 @@ sparc32_pseudo_register_write (struct gdbarch *gdbarch,
   regcache_raw_write (regcache, regnum + 1, buf + 4);
 }
 \f
+/* Implement "in_function_epilogue_p".  */
+
+int
+sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  /* This function must return true if we are one instruction after an
+     instruction that destroyed the stack frame of the current
+     function.  The SPARC instructions used to restore the callers
+     stack frame are RESTORE and RETURN/RETT.
+
+     Of these RETURN/RETT is a branch instruction and thus we return
+     true if we are in its delay slot.
+
+     RESTORE is almost always found in the delay slot of a branch
+     instruction that transfers control to the caller, such as JMPL.
+     Thus the next instruction is in the caller frame and we don't
+     need to do anything about it.  */
+
+  unsigned int insn = sparc_fetch_instruction (pc - 4);
+
+  return X_RETTURN (insn);
+}
+\f
 
 static CORE_ADDR
 sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
diff --git a/gdb/sparc-tdep.h b/gdb/sparc-tdep.h
index b83d711..a065ebe 100644
--- a/gdb/sparc-tdep.h
+++ b/gdb/sparc-tdep.h
@@ -193,6 +193,9 @@ extern struct sparc_frame_cache *
 extern struct sparc_frame_cache *
   sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
 
+extern int
+  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
+
 \f
 
 extern int sparc_software_single_step (struct frame_info *frame);
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 52958df..9e4db3a 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -1196,6 +1196,7 @@ sparc64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
     (gdbarch, default_stabs_argument_has_addr);
 
   set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
+  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
 
   /* Hook in the DWARF CFI frame unwinder.  */
   dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Mark Kettenis]

> From: jose.marchesi@oracle.com (Jose E. Marchesi)
> Date: Mon, 10 Feb 2014 13:40:23 +0100
> 
> 2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
> 
> 	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
> 	(X_RETTURN): New macro.
> 	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
> 
> 	* sparc64-tdep.c (sparc64_init_abi): Hook
> 	sparc_in_function_epilogue_p.

Finally managed to test this on OpenBSD/sparc64.  Seems to fix an
XFAIL and a FAIL in mi-watch.exp for me.  Although those are a bit
suspect since (software) watchpoints are currently a bit broken on
OpenBSD/sparc64.  Pretty sure this doesn't cause any regressions
though, so please go ahaed.



> diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
> index 38b345b..311a156 100644
> --- a/gdb/sparc-tdep.c
> +++ b/gdb/sparc-tdep.c
> @@ -88,6 +88,9 @@ struct regset;
>  #define X_DISP19(i) ((((i) & 0x7ffff) ^ 0x40000) - 0x40000)
>  #define X_DISP10(i) ((((((i) >> 11) && 0x300) | (((i) >> 5) & 0xff)) ^ 0x200) - 0x200)
>  #define X_SIMM13(i) ((((i) & 0x1fff) ^ 0x1000) - 0x1000)
> +/* Macros to identify some instructions.  */
> +/* RETURN (RETT in V8) */
> +#define X_RETTURN(i) ((X_OP (i) == 0x2) && (X_OP3 (i) == 0x39))
>  
>  /* Fetch the instruction at PC.  Instructions are always big-endian
>     even if the processor operates in little-endian mode.  */
> @@ -452,6 +455,29 @@ sparc32_pseudo_register_write (struct gdbarch *gdbarch,
>    regcache_raw_write (regcache, regnum + 1, buf + 4);
>  }
>  \f
> +/* Implement "in_function_epilogue_p".  */
> +
> +int
> +sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
> +{
> +  /* This function must return true if we are one instruction after an
> +     instruction that destroyed the stack frame of the current
> +     function.  The SPARC instructions used to restore the callers
> +     stack frame are RESTORE and RETURN/RETT.
> +
> +     Of these RETURN/RETT is a branch instruction and thus we return
> +     true if we are in its delay slot.
> +
> +     RESTORE is almost always found in the delay slot of a branch
> +     instruction that transfers control to the caller, such as JMPL.
> +     Thus the next instruction is in the caller frame and we don't
> +     need to do anything about it.  */
> +
> +  unsigned int insn = sparc_fetch_instruction (pc - 4);
> +
> +  return X_RETTURN (insn);
> +}
> +\f
>  
>  static CORE_ADDR
>  sparc32_frame_align (struct gdbarch *gdbarch, CORE_ADDR address)
> diff --git a/gdb/sparc-tdep.h b/gdb/sparc-tdep.h
> index b83d711..a065ebe 100644
> --- a/gdb/sparc-tdep.h
> +++ b/gdb/sparc-tdep.h
> @@ -193,6 +193,9 @@ extern struct sparc_frame_cache *
>  extern struct sparc_frame_cache *
>    sparc32_frame_cache (struct frame_info *this_frame, void **this_cache);
>  
> +extern int
> +  sparc_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc);
> +
>  \f
>  
>  extern int sparc_software_single_step (struct frame_info *frame);
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index 52958df..9e4db3a 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -1196,6 +1196,7 @@ sparc64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
>      (gdbarch, default_stabs_argument_has_addr);
>  
>    set_gdbarch_skip_prologue (gdbarch, sparc64_skip_prologue);
> +  set_gdbarch_in_function_epilogue_p (gdbarch, sparc_in_function_epilogue_p);
>  
>    /* Hook in the DWARF CFI frame unwinder.  */
>    dwarf2_frame_set_init_reg (gdbarch, sparc64_dwarf2_frame_init_reg);
> 

Re: [PATCH] add gdbarch_in_function_epilogue_p hook for sparc64

[Jose E. Marchesi]

    > From: jose.marchesi@oracle.com (Jose E. Marchesi)
    > Date: Mon, 10 Feb 2014 13:40:23 +0100
    > 
    > 2013-10-16  Jose E. Marchesi  <jose.marchesi@oracle.com>
    > 
    > 	* sparc-tdep.c (sparc_in_function_epilogue_p): New function.
    > 	(X_RETTURN): New macro.
    > 	* sparc-tdep.h: sparc_in_function_epilogue_p prototype.
    > 
    > 	* sparc64-tdep.c (sparc64_init_abi): Hook
    > 	sparc_in_function_epilogue_p.
    
    Finally managed to test this on OpenBSD/sparc64.  Seems to fix an
    XFAIL and a FAIL in mi-watch.exp for me.  Although those are a bit
    suspect since (software) watchpoints are currently a bit broken on
    OpenBSD/sparc64.  Pretty sure this doesn't cause any regressions
    though, so please go ahaed.

Committed.  Thanks.