Re: [PATCH 2/3] Kprobes: Define retval helper

Re: [PATCH 2/3] Kprobes: Define retval helper

[Christoph Hellwig]

On Wed, Aug 09, 2006 at 03:13:11PM +0530, Ananth N Mavinakayanahalli wrote:
> > Good idea.  You should add parentheses around regs, otherwise the C
> > preprocessor might bite users.  Also the shouting name is quite ugly.
> > In fact it should probably go to asm/system.h or similar and not have
> > a kprobes name - it just extracts the return value from a struct pt_regs
> > after all.
> 
> Done! How does this look? I added it to asm/ptrace.h so it lives along
> with the instruction_pointer() definition.

Looks good, but it would be much better if we had it for every single
architecture.  I've cc'ed linux-arch so the arch maintainers can comments.

Even if we don't manage to get every architecture maintainer to help out
you should at least add sparc and s390 to have full coverage of
architectures with kprobes support

> Add the "get_retval" macro that just extracts the return value given the
> pt_regs. Useful in situations such as while using function-return
> probes.
> 
> Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> 
> ---
>  include/asm-i386/ptrace.h    |    3 +++
>  include/asm-ia64/ptrace.h    |    3 +++
>  include/asm-powerpc/ptrace.h |    2 ++
>  include/asm-x86_64/ptrace.h  |    2 ++
>  4 files changed, 10 insertions(+)
> 
> Index: linux-2.6.18-rc3/include/asm-i386/ptrace.h
> ===================================================================
> --- linux-2.6.18-rc3.orig/include/asm-i386/ptrace.h
> +++ linux-2.6.18-rc3/include/asm-i386/ptrace.h
> @@ -79,7 +79,10 @@ static inline int user_mode_vm(struct pt
>  {
>  	return ((regs->xcs & 3) | (regs->eflags & VM_MASK)) != 0;
>  }
> +
>  #define instruction_pointer(regs) ((regs)->eip)
> +#define get_retval(regs) ((regs)->eax)
> +
>  #if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
>  extern unsigned long profile_pc(struct pt_regs *regs);
>  #else
> Index: linux-2.6.18-rc3/include/asm-ia64/ptrace.h
> ===================================================================
> --- linux-2.6.18-rc3.orig/include/asm-ia64/ptrace.h
> +++ linux-2.6.18-rc3/include/asm-ia64/ptrace.h
> @@ -237,6 +237,9 @@ struct switch_stack {
>   * the canonical representation by adding to instruction pointer.
>   */
>  # define instruction_pointer(regs) ((regs)->cr_iip + ia64_psr(regs)->ri)
> +
> +#define get_retval(regs) ((regs)->r8)
> +
>  /* Conserve space in histogram by encoding slot bits in address
>   * bits 2 and 3 rather than bits 0 and 1.
>   */
> Index: linux-2.6.18-rc3/include/asm-powerpc/ptrace.h
> ===================================================================
> --- linux-2.6.18-rc3.orig/include/asm-powerpc/ptrace.h
> +++ linux-2.6.18-rc3/include/asm-powerpc/ptrace.h
> @@ -73,6 +73,8 @@ struct pt_regs {
>  #ifndef __ASSEMBLY__
>  
>  #define instruction_pointer(regs) ((regs)->nip)
> +#define get_retval(regs) ((regs)->gpr[3])
> +
>  #ifdef CONFIG_SMP
>  extern unsigned long profile_pc(struct pt_regs *regs);
>  #else
> Index: linux-2.6.18-rc3/include/asm-x86_64/ptrace.h
> ===================================================================
> --- linux-2.6.18-rc3.orig/include/asm-x86_64/ptrace.h
> +++ linux-2.6.18-rc3/include/asm-x86_64/ptrace.h
> @@ -84,6 +84,8 @@ struct pt_regs {
>  #define user_mode(regs) (!!((regs)->cs & 3))
>  #define user_mode_vm(regs) user_mode(regs)
>  #define instruction_pointer(regs) ((regs)->rip)
> +#define get_retval(regs) ((regs)->rax)
> +
>  extern unsigned long profile_pc(struct pt_regs *regs);
>  void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
>  
---end quoted text---

Re: [PATCH 2/3] Kprobes: Define retval helper

[Andi Kleen]

> >  #define instruction_pointer(regs) ((regs)->eip)
> > +#define get_retval(regs) ((regs)->eax)

return_value() would match the names of the existing macro better

-Andi

Re: [PATCH 2/3] Kprobes: Define retval helper

[Ananth N Mavinakayanahalli]

On Wed, Aug 09, 2006 at 11:55:50AM +0200, Andi Kleen wrote:
> 
> > >  #define instruction_pointer(regs) ((regs)->eip)
> > > +#define get_retval(regs) ((regs)->eax)
> 
> return_value() would match the names of the existing macro better
 
Updated patch ...

Add the "return_value" macro that just extracts the return value given
the pt_regs. Useful in situations such as while using function-return
probes.


Signed-off-by: Ananth N Mavinakayanahalli <ananth@in.ibm.com>

---
 include/asm-i386/ptrace.h    |    3 +++
 include/asm-ia64/ptrace.h    |    3 +++
 include/asm-powerpc/ptrace.h |    2 ++
 include/asm-x86_64/ptrace.h  |    2 ++
 4 files changed, 10 insertions(+)

Index: linux-2.6.18-rc3/include/asm-i386/ptrace.h
===================================================================
--- linux-2.6.18-rc3.orig/include/asm-i386/ptrace.h
+++ linux-2.6.18-rc3/include/asm-i386/ptrace.h
@@ -79,7 +79,10 @@ static inline int user_mode_vm(struct pt
 {
 	return ((regs->xcs & 3) | (regs->eflags & VM_MASK)) != 0;
 }
+
 #define instruction_pointer(regs) ((regs)->eip)
+#define return_value(regs) ((regs)->eax)
+
 #if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
Index: linux-2.6.18-rc3/include/asm-ia64/ptrace.h
===================================================================
--- linux-2.6.18-rc3.orig/include/asm-ia64/ptrace.h
+++ linux-2.6.18-rc3/include/asm-ia64/ptrace.h
@@ -237,6 +237,9 @@ struct switch_stack {
  * the canonical representation by adding to instruction pointer.
  */
 # define instruction_pointer(regs) ((regs)->cr_iip + ia64_psr(regs)->ri)
+
+#define return_value(regs) ((regs)->r8)
+
 /* Conserve space in histogram by encoding slot bits in address
  * bits 2 and 3 rather than bits 0 and 1.
  */
Index: linux-2.6.18-rc3/include/asm-powerpc/ptrace.h
===================================================================
--- linux-2.6.18-rc3.orig/include/asm-powerpc/ptrace.h
+++ linux-2.6.18-rc3/include/asm-powerpc/ptrace.h
@@ -73,6 +73,8 @@ struct pt_regs {
 #ifndef __ASSEMBLY__
 
 #define instruction_pointer(regs) ((regs)->nip)
+#define return_value(regs) ((regs)->gpr[3])
+
 #ifdef CONFIG_SMP
 extern unsigned long profile_pc(struct pt_regs *regs);
 #else
Index: linux-2.6.18-rc3/include/asm-x86_64/ptrace.h
===================================================================
--- linux-2.6.18-rc3.orig/include/asm-x86_64/ptrace.h
+++ linux-2.6.18-rc3/include/asm-x86_64/ptrace.h
@@ -84,6 +84,8 @@ struct pt_regs {
 #define user_mode(regs) (!!((regs)->cs & 3))
 #define user_mode_vm(regs) user_mode(regs)
 #define instruction_pointer(regs) ((regs)->rip)
+#define return_value(regs) ((regs)->rax)
+
 extern unsigned long profile_pc(struct pt_regs *regs);
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
 

Re: [PATCH 2/3] Kprobes: Define retval helper

[Martin Schwidefsky]

On Wed, 2006-08-09 at 15:49 +0530, Ananth N Mavinakayanahalli wrote:
> Updated patch ...
> 
> Add the "return_value" macro that just extracts the return value given
> the pt_regs. Useful in situations such as while using function-return
> probes.

return_value definition for s390 see below.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

---

diff -urpN linux-2.6.18-rc3/include/asm-s390/ptrace.h linux-2.6.18-s390/include/asm-s390/ptrace.h
--- linux-2.6.18-rc3/include/asm-s390/ptrace.h	2006-06-29 12:47:32.000000000 +0200
+++ linux-2.6.18-s390/include/asm-s390/ptrace.h	2006-08-09 12:25:23.000000000 +0200
@@ -472,6 +472,7 @@ struct user_regs_struct
 
 #define user_mode(regs) (((regs)->psw.mask & PSW_MASK_PSTATE) != 0)
 #define instruction_pointer(regs) ((regs)->psw.addr & PSW_ADDR_INSN)
+#define return_value(regs)((regs)->gprs[2])
 #define profile_pc(regs) instruction_pointer(regs)
 extern void show_regs(struct pt_regs * regs);
 #endif

Re: [PATCH 2/3] Kprobes: Define retval helper

[David Howells]

Christoph Hellwig <hch@infradead.org> wrote:

> > > Good idea.  You should add parentheses around regs, otherwise the C
> > > preprocessor might bite users.  Also the shouting name is quite ugly.
> > > In fact it should probably go to asm/system.h or similar and not have
> > > a kprobes name - it just extracts the return value from a struct pt_regs
> > > after all.
> > 
> > Done! How does this look? I added it to asm/ptrace.h so it lives along
> > with the instruction_pointer() definition.

I presume we don't care about return values that span multiple registers - for
instance if you return a 64-bit value on i386 it'll wind up in EDX:EAX.

David

Re: [PATCH 2/3] Kprobes: Define retval helper

[Ananth N Mavinakayanahalli]

On Wed, Aug 09, 2006 at 02:16:04PM +0100, David Howells wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > > > Good idea.  You should add parentheses around regs, otherwise the C
> > > > preprocessor might bite users.  Also the shouting name is quite ugly.
> > > > In fact it should probably go to asm/system.h or similar and not have
> > > > a kprobes name - it just extracts the return value from a struct pt_regs
> > > > after all.
> > > 
> > > Done! How does this look? I added it to asm/ptrace.h so it lives along
> > > with the instruction_pointer() definition.
> 
> I presume we don't care about return values that span multiple registers - for
> instance if you return a 64-bit value on i386 it'll wind up in EDX:EAX.

Yes. This helper is mostly to address the common case, not the 64-bit
one.

Ananth