[PATCH] common/symbols: Drop '+0/<len>' when printing function pointer symbols.

[PATCH] common/symbols: Drop '+0/<len>' when printing function pointer symbols.

[Andrew Cooper]

Introduce print_function() with the same semantics as print_symbol().  The
underlying __print_symbol() now takes an extra boolean indicating whether we
are expecting to print a function pointer.   In the case that we are expecting
a function pointer, and the offset is 0, drop the offset and length.

The requirement for offset being 0 is for the (hopefully never, but we really
want to know if it does happen) case where a Xen function pointer is not
actually pointing at the start of a function.

The relevent print_symbol() functions are updated to print_function()

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/irq.c        |    2 +-
 xen/arch/x86/time.c       |    4 ++--
 xen/common/symbols.c      |    7 +++++--
 xen/common/timer.c        |    2 +-
 xen/include/xen/symbols.h |   16 +++++++++++++---
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index c61cc46..c349dd2 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2280,7 +2280,7 @@ static void dump_irqs(unsigned char key)
             printk("\n");
         }
         else if ( desc->action )
-            print_symbol("%s\n", (unsigned long)desc->action->handler);
+            print_function("%s()\n", (unsigned long)desc->action->handler);
         else
             printk("mapped, unbound\n");
 
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index c1bbd50..d857650 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1475,8 +1475,8 @@ static int _disable_pit_irq(void(*hpet_broadcast_setup)(void))
         {
             if ( xen_cpuidle > 0 )
             {
-                print_symbol("%s() failed, turning to PIT broadcast\n",
-                             (unsigned long)hpet_broadcast_setup);
+                print_function("%s() failed, turning to PIT broadcast\n",
+                               (unsigned long)hpet_broadcast_setup);
                 return -1;
             }
             ret = 0;
diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index 83b2b58..9dcaaba 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -150,7 +150,7 @@ const char *symbols_lookup(unsigned long addr,
 }
 
 /* Replace "%s" in format with address, or returns -errno. */
-void __print_symbol(const char *fmt, unsigned long address)
+void __print_symbol(const char *fmt, unsigned long address, bool_t fnptr)
 {
     const char *name;
     unsigned long offset, size, flags;
@@ -168,7 +168,10 @@ void __print_symbol(const char *fmt, unsigned long address)
     if (!name)
         snprintf(buffer, BUFFER_SIZE, "???");
     else
-        snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset, size);
+        if ( fnptr && offset == 0 )
+            snprintf(buffer, BUFFER_SIZE, "%s", name);
+        else
+            snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset, size);
 
     printk(fmt, buffer);
 
diff --git a/xen/common/timer.c b/xen/common/timer.c
index 9ed74e9..2008aa3 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -513,7 +513,7 @@ static void dump_timer(struct timer *t, s_time_t now)
 {
     printk("  ex=%8"PRId64"us timer=%p cb=%p(%p)",
            (t->expires - now) / 1000, t, t->function, t->data);
-    print_symbol(" %s\n", (unsigned long)t->function);
+    print_function(" %s()\n", (unsigned long)t->function);
 }
 
 static void dump_timerq(unsigned char key)
diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
index 37cf6bf..866231e 100644
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -11,8 +11,12 @@ const char *symbols_lookup(unsigned long addr,
                            unsigned long *offset,
                            char *namebuf);
 
-/* Replace "%s" in format with address, if found */
-void __print_symbol(const char *fmt, unsigned long address);
+/*
+ * Replace "%s" in format with address, if found.  'fnptr' indicates a
+ * function pointer address is expected, in which case the '+0/<length>' is
+ * dropped for clarity.
+ */
+void __print_symbol(const char *fmt, unsigned long address, bool_t fnptr);
 
 /* This macro allows us to keep printk typechecking */
 static void __check_printsym_format(const char *fmt, ...)
@@ -31,7 +35,13 @@ static void __check_printsym_format(const char *fmt, ...)
 #define print_symbol(fmt, addr)			\
 do {						\
 	__check_printsym_format(fmt, "");	\
-	__print_symbol(fmt, addr);		\
+	__print_symbol(fmt, addr, 0);		\
+} while(0)
+
+#define print_function(fmt, addr)		\
+do {						\
+	__check_printsym_format(fmt, "");	\
+	__print_symbol(fmt, addr, 1);           \
 } while(0)
 
 #endif /*_XEN_SYMBOLS_H*/
-- 
1.7.10.4

Re: [PATCH] common/symbols: Drop '+0/<len>' when printing function pointer symbols.

[Ian Campbell]

On Fri, 2013-10-04 at 11:02 +0100, Andrew Cooper wrote:
> Introduce print_function() with the same semantics as print_symbol().  The
> underlying __print_symbol() now takes an extra boolean indicating whether we
> are expecting to print a function pointer.   In the case that we are expecting
> a function pointer, and the offset is 0, drop the offset and length.
> 
> The requirement for offset being 0 is for the (hopefully never, but we really
> want to know if it does happen) case where a Xen function pointer is not
> actually pointing at the start of a function.
> 
> The relevent print_symbol() functions are updated to print_function()

I've been yearning after the Linux printk %pS and %pF extensions for a
while now...

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  xen/arch/x86/irq.c        |    2 +-
>  xen/arch/x86/time.c       |    4 ++--
>  xen/common/symbols.c      |    7 +++++--
>  xen/common/timer.c        |    2 +-
>  xen/include/xen/symbols.h |   16 +++++++++++++---
>  5 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index c61cc46..c349dd2 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2280,7 +2280,7 @@ static void dump_irqs(unsigned char key)
>              printk("\n");
>          }
>          else if ( desc->action )
> -            print_symbol("%s\n", (unsigned long)desc->action->handler);
> +            print_function("%s()\n", (unsigned long)desc->action->handler);
>          else
>              printk("mapped, unbound\n");
>  
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index c1bbd50..d857650 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1475,8 +1475,8 @@ static int _disable_pit_irq(void(*hpet_broadcast_setup)(void))
>          {
>              if ( xen_cpuidle > 0 )
>              {
> -                print_symbol("%s() failed, turning to PIT broadcast\n",
> -                             (unsigned long)hpet_broadcast_setup);
> +                print_function("%s() failed, turning to PIT broadcast\n",
> +                               (unsigned long)hpet_broadcast_setup);
>                  return -1;
>              }
>              ret = 0;
> diff --git a/xen/common/symbols.c b/xen/common/symbols.c
> index 83b2b58..9dcaaba 100644
> --- a/xen/common/symbols.c
> +++ b/xen/common/symbols.c
> @@ -150,7 +150,7 @@ const char *symbols_lookup(unsigned long addr,
>  }
>  
>  /* Replace "%s" in format with address, or returns -errno. */
> -void __print_symbol(const char *fmt, unsigned long address)
> +void __print_symbol(const char *fmt, unsigned long address, bool_t fnptr)
>  {
>      const char *name;
>      unsigned long offset, size, flags;
> @@ -168,7 +168,10 @@ void __print_symbol(const char *fmt, unsigned long address)
>      if (!name)
>          snprintf(buffer, BUFFER_SIZE, "???");
>      else
> -        snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset, size);
> +        if ( fnptr && offset == 0 )
> +            snprintf(buffer, BUFFER_SIZE, "%s", name);
> +        else
> +            snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset, size);
>  
>      printk(fmt, buffer);
>  
> diff --git a/xen/common/timer.c b/xen/common/timer.c
> index 9ed74e9..2008aa3 100644
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -513,7 +513,7 @@ static void dump_timer(struct timer *t, s_time_t now)
>  {
>      printk("  ex=%8"PRId64"us timer=%p cb=%p(%p)",
>             (t->expires - now) / 1000, t, t->function, t->data);
> -    print_symbol(" %s\n", (unsigned long)t->function);
> +    print_function(" %s()\n", (unsigned long)t->function);
>  }
>  
>  static void dump_timerq(unsigned char key)
> diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
> index 37cf6bf..866231e 100644
> --- a/xen/include/xen/symbols.h
> +++ b/xen/include/xen/symbols.h
> @@ -11,8 +11,12 @@ const char *symbols_lookup(unsigned long addr,
>                             unsigned long *offset,
>                             char *namebuf);
>  
> -/* Replace "%s" in format with address, if found */
> -void __print_symbol(const char *fmt, unsigned long address);
> +/*
> + * Replace "%s" in format with address, if found.  'fnptr' indicates a
> + * function pointer address is expected, in which case the '+0/<length>' is
> + * dropped for clarity.
> + */
> +void __print_symbol(const char *fmt, unsigned long address, bool_t fnptr);
>  
>  /* This macro allows us to keep printk typechecking */
>  static void __check_printsym_format(const char *fmt, ...)
> @@ -31,7 +35,13 @@ static void __check_printsym_format(const char *fmt, ...)
>  #define print_symbol(fmt, addr)			\
>  do {						\
>  	__check_printsym_format(fmt, "");	\
> -	__print_symbol(fmt, addr);		\
> +	__print_symbol(fmt, addr, 0);		\
> +} while(0)
> +
> +#define print_function(fmt, addr)		\
> +do {						\
> +	__check_printsym_format(fmt, "");	\
> +	__print_symbol(fmt, addr, 1);           \
>  } while(0)
>  
>  #endif /*_XEN_SYMBOLS_H*/

Re: [PATCH] common/symbols: Drop '+0/<len>' when printing function pointer symbols.

[Jan Beulich]

>>> On 04.10.13 at 12:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Introduce print_function() with the same semantics as print_symbol().  The
> underlying __print_symbol() now takes an extra boolean indicating whether we
> are expecting to print a function pointer.   In the case that we are 
> expecting
> a function pointer, and the offset is 0, drop the offset and length.
> 
> The requirement for offset being 0 is for the (hopefully never, but we 
> really
> want to know if it does happen) case where a Xen function pointer is not
> actually pointing at the start of a function.
> 
> The relevent print_symbol() functions are updated to print_function()

There's no reason why the same couldn't apply to data symbols.
Rather than doing it this way (and with a mis-named function), I'd
much prefer going the printk() format string extensions route that
Linux went. This would at once allow re-combining the broken up
printing when symbols are involved into single invocations of printk().
This has been on my (mental) to-do list for quite some time, but
hasn't been important enough for me to ever get to it.

Jan

Re: [PATCH] common/symbols: Drop '+0/<len>' when printing function pointer symbols.

[Andrew Cooper]

On 04/10/13 11:15, Jan Beulich wrote:
>>>> On 04.10.13 at 12:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Introduce print_function() with the same semantics as print_symbol().  The
>> underlying __print_symbol() now takes an extra boolean indicating whether we
>> are expecting to print a function pointer.   In the case that we are 
>> expecting
>> a function pointer, and the offset is 0, drop the offset and length.
>>
>> The requirement for offset being 0 is for the (hopefully never, but we 
>> really
>> want to know if it does happen) case where a Xen function pointer is not
>> actually pointing at the start of a function.
>>
>> The relevent print_symbol() functions are updated to print_function()
> There's no reason why the same couldn't apply to data symbols.
> Rather than doing it this way (and with a mis-named function), I'd
> much prefer going the printk() format string extensions route that
> Linux went. This would at once allow re-combining the broken up
> printing when symbols are involved into single invocations of printk().
> This has been on my (mental) to-do list for quite some time, but
> hasn't been important enough for me to ever get to it.
>
> Jan
>

Right - that is two very quick recommendations for %pS and %pF.  I will
see what I can do in my next bit of free time.  (FWIW, this patch came
about from frustration during my HPET interrupt debugging work, and I
was looking for a momentary break from the HPET code itself)

~Andrew