Andrew Cooper
2013-Oct-04 10:02 UTC
[PATCH] common/symbols: Drop ''+0/<len>'' when printing function pointer symbols.
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
Ian Campbell
2013-Oct-04 10:12 UTC
Re: [PATCH] common/symbols: Drop ''+0/<len>'' when printing function pointer symbols.
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*/
Jan Beulich
2013-Oct-04 10:15 UTC
Re: [PATCH] common/symbols: Drop ''+0/<len>'' when printing function pointer symbols.
>>> 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
Andrew Cooper
2013-Oct-04 10:18 UTC
Re: [PATCH] common/symbols: Drop ''+0/<len>'' when printing function pointer symbols.
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