thr3ads.net - Xen devel - [PATCH v2 0/7] Printk symbol specifier [Nov 2013]

If this information is useful, please help other people find it:
Share via:

Andrew Cooper

2013-Nov-04 21:30 UTC

[PATCH v2 0/7] Printk symbol specifier

This series implements %ps and %pS, in the same way as Linux.

Changes from v1:
  The first patch has grown to three.  The breakdown is now:

The first two patches refactor string() and pointer() respectivly out of
vsnprintf().  This is just code motion in preparation for the subsequent
patch.

The third patch is the main implementation of %ps and %pS.  With the
refactoring from the first patch, the recursive call to scnprintf in v1 can be
removed.

One inconsistency with the behaviour of print_symbol() is that in the case
that a symbol cant be found, the pointer is printed as per %p, rather than
"???" as before.

Documentation is now provided in docs/misc/printk-formats.txt

The fourth and fifth patches replace all uses of print_symbol() with %ps/%pS,
for x86 and arm respectively.  Most replacements are straight replacements,
but I have taken the opportunity to slightly cleanup the stack tracing code.
When two CPUs are racing at printing a stack, they contend on the console
spinlock, resulting in interleaving across end of the partial strings.  Now,
each full line of the stack trace is printed from a single printk(), so the
interleaving will occur at the line boundaries rather than mid-line
boundaries.

The sixth patch removes print_symbol() and friends, now that the functionality
has been completely replaced.

The seventh patch is not intended for committing, but for people wishing to
test and verify some of the boundary conditions.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>

-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 1/7] common/vsprintf: Refactor string() out of vsnprintf()

No fuctional change

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/vsprintf.c |   49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 95bf85d..1bcbd79 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -235,6 +235,32 @@ static char *number(
     return buf;
 }
 
+static char *string(char *str, char *end, const char *s,
+                    int field_width, int precision, int flags)
+{
+    int i, len = strnlen(s, precision);
+
+    if (!(flags & LEFT)) {
+        while (len < field_width--) {
+            if (str <= end)
+                *str = '' '';
+            ++str;
+        }
+    }
+    for (i = 0; i < len; ++i) {
+        if (str <= end)
+            *str = *s;
+        ++str; ++s;
+    }
+    while (len < field_width--) {
+        if (str <= end)
+            *str = '' '';
+        ++str;
+    }
+
+    return str;
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -255,9 +281,8 @@ static char *number(
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-    int len;
     unsigned long long num;
-    int i, base;
+    int base;
     char *str, *end, c;
     const char *s;
 
@@ -370,25 +395,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
             if ((unsigned long)s < PAGE_SIZE)
                 s = "<NULL>";
 
-            len = strnlen(s, precision);
-
-            if (!(flags & LEFT)) {
-                while (len < field_width--) {
-                    if (str <= end)
-                        *str = '' '';
-                    ++str;
-                }
-            }
-            for (i = 0; i < len; ++i) {
-                if (str <= end)
-                    *str = *s;
-                ++str; ++s;
-            }
-            while (len < field_width--) {
-                if (str <= end)
-                    *str = '' '';
-                ++str;
-            }
+            str = string(str, end, s, field_width, precision, flags);
             continue;
 
         case ''p'':
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

No fuctional change

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/vsprintf.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 1bcbd79..5fdd391 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -261,6 +261,21 @@ static char *string(char *str, char *end, const char *s,
     return str;
 }
 
+static char *pointer(char *str, char *end,
+                     unsigned long val, int field_width, int precision,
+                     int flags)
+{
+    if ( field_width == -1 )
+    {
+        field_width = 2 * sizeof(void *);
+        flags |= ZEROPAD;
+    }
+
+    str = number(str, end, val, 16, field_width, precision, flags);
+
+    return str;
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into
@@ -399,13 +414,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
             continue;
 
         case ''p'':
-            if (field_width == -1) {
-                field_width = 2*sizeof(void *);
-                flags |= ZEROPAD;
-            }
-            str = number(str, end,
-                         (unsigned long) va_arg(args, void *),
-                         16, field_width, precision, flags);
+            str = pointer(str, end, (unsigned long) va_arg(args, void *),
+                          field_width, precision, flags);
             continue;
 
 
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

Introduce the %ps and %pS format options for printing a symbol.

  %ps will print the symbol name alone
  %pS will print the symbol name + offset / size

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 docs/misc/printk-formats.txt |   11 ++++++++
 xen/common/vsprintf.c        |   58 +++++++++++++++++++++++++++++++++++++-----
 2 files changed, 62 insertions(+), 7 deletions(-)
 create mode 100644 docs/misc/printk-formats.txt

diff --git a/docs/misc/printk-formats.txt b/docs/misc/printk-formats.txt
new file mode 100644
index 0000000..ef25e16
--- /dev/null
+++ b/docs/misc/printk-formats.txt
@@ -0,0 +1,11 @@
+Xen custom %p format options.  A subset, borrowed from Linux.
+
+All parameters to a %p option should be enclosed with the _p() macro
+
+Symbol/Function pointers:
+
+       %ps     Symbol name only
+       %pS     Symbol name + offset / function length
+
+       In the case that an appropriate symbol name can''t be found,
%p[sS] will
+       fall back to ''%p'' and print the address in hex.
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 5fdd391..5ac5b9b 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -17,6 +17,7 @@
  */
 
 #include <xen/ctype.h>
+#include <xen/symbols.h>
 #include <xen/lib.h>
 #include <asm/div64.h>
 #include <asm/page.h>
@@ -261,19 +262,61 @@ static char *string(char *str, char *end, const char *s,
     return str;
 }
 
-static char *pointer(char *str, char *end,
+static char *pointer(char *str, char *end, const char **fmt_ptr,
                      unsigned long val, int field_width, int precision,
                      int flags)
 {
-    if ( field_width == -1 )
+    const char *fmt = *fmt_ptr, *s;
+
+    /*
+     * Custom %p suffixes, compatible with Linux.
+     * See XEN_ROOT/docs/misc/printk-formats.txt
+     */
+    switch ( fmt[1] )
     {
-        field_width = 2 * sizeof(void *);
-        flags |= ZEROPAD;
+    case ''s'': /* Symbol name */
+    case ''S'': /* Symbol name with offset and size */
+    {
+        unsigned long sym_size, sym_offset;
+        char namebuf[KSYM_NAME_LEN+1];
+
+        /* Advance fmt pointer, as we have consumed ''s'' or
''S''.  Leave fmt
+         * along for consistency. */
+        (*fmt_ptr)++;
+
+        s = symbols_lookup(val, &sym_size, &sym_offset, namebuf);
+
+        /* If the symbol is not found, fall back to printing the address */
+        if ( !s )
+            goto regular_pointer;
+
+        /* Print symbol name */
+        str = string(str, end, s, -1, -1, 0);
+
+        if ( fmt[1] == ''S'' )
+        {
+            /* Print ''+0x<offset>/0x<len>'' */
+            str = string(str, end, "+0x", -1, -1, 0);
+            str = number(str, end, sym_offset, 16, -1, -1, 0);
+            str = string(str, end, "/0x", -1, -1, 0);
+            str = number(str, end, sym_size, 16, -1, -1, 0);
+        }
+
+        return str;
     }
 
-    str = number(str, end, val, 16, field_width, precision, flags);
+    default:
+    regular_pointer:
+
+        if ( field_width == -1 )
+        {
+            field_width = 2 * sizeof(void *);
+            flags |= ZEROPAD;
+        }
+
+        return number(str, end, val, 16, field_width, precision, flags);
+    }
 
-    return str;
 }
 
 /**
@@ -414,7 +457,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
             continue;
 
         case ''p'':
-            str = pointer(str, end, (unsigned long) va_arg(args, void *),
+            /* pointer() might advance fmt (%pS for example) */
+            str = pointer(str, end, &fmt, (unsigned long) va_arg(args, void
*),
                           field_width, precision, flags);
             continue;
 
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

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          |    7 ++-----
 xen/arch/x86/time.c         |    3 +--
 xen/arch/x86/traps.c        |   30 ++++++++++--------------------
 xen/arch/x86/x86_64/traps.c |    2 +-
 xen/common/timer.c          |    6 +++---
 5 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index a984bda..c2e5629 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2276,7 +2276,7 @@ static void dump_irqs(unsigned char key)
             printk("\n");
         }
         else if ( desc->action )
-            print_symbol("%s\n", (unsigned
long)desc->action->handler);
+            printk("%ps()\n", _p(desc->action->handler));
         else
             printk("mapped, unbound\n");
 
@@ -2288,10 +2288,7 @@ static void dump_irqs(unsigned char key)
     printk("Direct vector information:\n");
     for ( i = FIRST_DYNAMIC_VECTOR; i < NR_VECTORS; ++i )
         if ( direct_apic_vector[i] )
-        {
-            printk("   %#02x -> ", i);
-            print_symbol("%s\n", (unsigned
long)direct_apic_vector[i]);
-        }
+            printk("   %#02x -> %ps()\n", i,
_p(direct_apic_vector[i]));
 
     dump_ioapic_irq_info();
 }
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index c1bbd50..daade00 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1475,8 +1475,7 @@ 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);
+                printk("hpet_broadcast_setup() failed, turning to PIT
broadcast\n");
                 return -1;
             }
             ret = 0;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 77c200b..0e3c6e3 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -198,19 +198,14 @@ static void show_trace(struct cpu_user_regs *regs)
 {
     unsigned long *stack = ESP_BEFORE_EXCEPTION(regs), addr;
 
-    printk("Xen call trace:\n   ");
-
-    printk("[<%p>]", _p(regs->eip));
-    print_symbol(" %s\n   ", regs->eip);
+    printk("Xen call trace:\n"
+           "   [<%p>] %pS\n", _p(regs->eip),
_p(regs->eip));
 
     while ( ((long)stack & (STACK_SIZE-BYTES_PER_LONG)) != 0 )
     {
         addr = *stack++;
         if ( is_active_kernel_text(addr) )
-        {
-            printk("[<%p>]", _p(addr));
-            print_symbol(" %s\n   ", addr);
-        }
+            printk("   [<%p>] %pS\n", _p(addr), _p(addr));
     }
 
     printk("\n");
@@ -222,8 +217,6 @@ static void show_trace(struct cpu_user_regs *regs)
 {
     unsigned long *frame, next, addr, low, high;
 
-    printk("Xen call trace:\n   ");
-
     /*
      * If RIP is not pointing into hypervisor code then someone may have
      * called into oblivion. Peek to see if they left a return address at
@@ -232,8 +225,9 @@ static void show_trace(struct cpu_user_regs *regs)
     addr = is_active_kernel_text(regs->eip) ||
            !is_active_kernel_text(*ESP_BEFORE_EXCEPTION(regs)) ?
            regs->eip : *ESP_BEFORE_EXCEPTION(regs);
-    printk("[<%p>]", _p(addr));
-    print_symbol(" %s\n   ", addr);
+
+    printk("Xen call trace:\n"
+           "   [<%p>] %pS\n", _p(addr), _p(addr));
 
     /* Bounds for range of valid frame pointer. */
     low  = (unsigned long)(ESP_BEFORE_EXCEPTION(regs) - 2);
@@ -269,8 +263,7 @@ static void show_trace(struct cpu_user_regs *regs)
             addr  = frame[1];
         }
 
-        printk("[<%p>]", _p(addr));
-        print_symbol(" %s\n   ", addr);
+        printk("   [<%p>] %pS\n", _p(addr), _p(addr));
 
         low = (unsigned long)&frame[2];
     }
@@ -338,10 +331,7 @@ void show_stack_overflow(unsigned int cpu, unsigned long
esp)
     {
         addr = *stack++;
         if ( is_active_kernel_text(addr) )
-        {
-            printk("%p: [<%p>]", stack, _p(addr));
-            print_symbol(" %s\n   ", addr);
-        }
+            printk("%p: [<%p>] %pS\n", stack, _p(addr),
_p(addr));
     }
 
     printk("\n");
@@ -3755,8 +3745,8 @@ void asm_domain_crash_synchronous(unsigned long addr)
     if ( addr == 0 )
         addr = this_cpu(last_extable_addr);
 
-    printk("domain_crash_sync called from entry.S: fault at %p ",
_p(addr));
-    print_symbol("%s\n", addr);
+    printk("domain_crash_sync called from entry.S: fault at %p
%pS\n",
+           _p(addr), _p(addr));
 
     __domain_crash_synchronous();
 }
diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index 0316d7c..ae93539 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -49,7 +49,7 @@ static void _show_registers(
 
     printk("RIP:    %04x:[<%016lx>]", regs->cs,
regs->rip);
     if ( context == CTXT_hypervisor )
-        print_symbol(" %s", regs->rip);
+        printk(" %pS", _p(regs->rip));
     printk("\nRFLAGS: %016lx   ", regs->rflags);
     if ( (context == CTXT_pv_guest) && v && v->vcpu_info )
         printk("EM: %d   ", !!vcpu_info(v, evtchn_upcall_mask));
diff --git a/xen/common/timer.c b/xen/common/timer.c
index 9ed74e9..eff9c26 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -511,9 +511,9 @@ s_time_t align_timer(s_time_t firsttick, uint64_t period)
 
 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);
+    printk("  ex=%8"PRId64"us timer=%p cb=%p(%p) %ps()\n",
+           (t->expires - now) / 1000, t, t->function, t->data,
+           _p(t->function));
 }
 
 static void dump_timerq(unsigned char key)
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 5/7] arm: Replace print_symbol() with new %ps/%pS format

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/arm/traps.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 287dd7b..10ee498 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -387,7 +387,7 @@ static void show_registers_32(struct cpu_user_regs *regs,
 #else
     printk("PC:     %08"PRIx32, regs->pc);
     if ( !guest_mode )
-        print_symbol(" %s", regs->pc);
+        printk(" %pS", _p(regs->pc));
     printk("\n");
 #endif
     printk("CPSR:   %08"PRIx32" MODE:%s\n", regs->cpsr,
@@ -462,7 +462,7 @@ static void show_registers_64(struct cpu_user_regs *regs,
 
     printk("PC:     %016"PRIx64, regs->pc);
     if ( !guest_mode )
-        print_symbol(" %s", regs->pc);
+        printk(" %pS", _p(regs->pc));
     printk("\n");
     printk("LR:     %016"PRIx64"\n", regs->lr);
     if ( guest_mode )
@@ -735,12 +735,10 @@ static void show_trace(struct cpu_user_regs *regs)
 {
     register_t *frame, next, addr, low, high;
 
-    printk("Xen call trace:\n   ");
+    printk("Xen call trace:\n");
 
-    printk("[<%p>]", _p(regs->pc));
-    print_symbol(" %s (PC)\n   ", regs->pc);
-    printk("[<%p>]", _p(regs->lr));
-    print_symbol(" %s (LR)\n   ", regs->lr);
+    printk("   [<%p>] %pS (PC)\n", _p(regs->pc),
_p(regs->pc));
+    printk("   [<%p>] %pS (LR)\n", _p(regs->lr),
_p(regs->lr));
 
     /* Bounds for range of valid frame pointer. */
     low  = (register_t)(STACK_BEFORE_EXCEPTION(regs));
@@ -760,8 +758,7 @@ static void show_trace(struct cpu_user_regs *regs)
         next  = frame[0];
         addr  = frame[1];
 
-        printk("[<%p>]", _p(addr));
-        print_symbol(" %s\n   ", addr);
+        printk("   [<%p>] %pS\n", _p(addr), _p(addr));
 
         low = (register_t)&frame[1];
     }
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 6/7] common/symbols: Remove print_symbol() and associated infrastructure

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/symbols.c      |   26 --------------------------
 xen/include/xen/symbols.h |   23 -----------------------
 2 files changed, 49 deletions(-)

diff --git a/xen/common/symbols.c b/xen/common/symbols.c
index 83b2b58..45941e1 100644
--- a/xen/common/symbols.c
+++ b/xen/common/symbols.c
@@ -148,29 +148,3 @@ const char *symbols_lookup(unsigned long addr,
     *offset = addr - symbols_address(low);
     return namebuf;
 }
-
-/* Replace "%s" in format with address, or returns -errno. */
-void __print_symbol(const char *fmt, unsigned long address)
-{
-    const char *name;
-    unsigned long offset, size, flags;
-
-    static DEFINE_SPINLOCK(lock);
-    static char namebuf[KSYM_NAME_LEN+1];
-#define BUFFER_SIZE sizeof("%s+%#lx/%#lx [%s]") + KSYM_NAME_LEN + \
-			2*(BITS_PER_LONG*3/10) + 1
-    static char buffer[BUFFER_SIZE];
-
-    spin_lock_irqsave(&lock, flags);
-
-    name = symbols_lookup(address, &size, &offset, namebuf);
-
-    if (!name)
-        snprintf(buffer, BUFFER_SIZE, "???");
-    else
-        snprintf(buffer, BUFFER_SIZE, "%s+%#lx/%#lx", name, offset,
size);
-
-    printk(fmt, buffer);
-
-    spin_unlock_irqrestore(&lock, flags);
-}
diff --git a/xen/include/xen/symbols.h b/xen/include/xen/symbols.h
index 37cf6bf..87cd77d 100644
--- a/xen/include/xen/symbols.h
+++ b/xen/include/xen/symbols.h
@@ -11,27 +11,4 @@ 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);
-
-/* This macro allows us to keep printk typechecking */
-static void __check_printsym_format(const char *fmt, ...)
-    __attribute__((format(printf,1,2)));
-    static inline void __check_printsym_format(const char *fmt, ...)
-{
-}
-
-#if 0
-#define print_fn_descriptor_symbol(fmt, addr)	\
-	print_symbol(fmt, *(unsigned long *)addr)
-#else
-#define print_fn_descriptor_symbol(fmt, addr) print_symbol(fmt, addr)
-#endif
-
-#define print_symbol(fmt, addr)			\
-do {						\
-	__check_printsym_format(fmt, "");	\
-	__print_symbol(fmt, addr);		\
-} while(0)
-
 #endif /*_XEN_SYMBOLS_H*/
-- 
1.7.10.4

Andrew Cooper

2013-Nov-04 21:30 UTC

head link

[PATCH 7/7] Test harness for new printk formatting

---
 xen/arch/x86/irq.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index c2e5629..68a956c 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2485,3 +2485,39 @@ bool_t hvm_domain_use_pirq(const struct domain *d, const
struct pirq *pirq)
     return is_hvm_domain(d) && pirq &&
            pirq->arch.hvm.emuirq != IRQ_UNBOUND; 
 }
+
+static void test_printk(unsigned char key)
+{
+    char buffer[8] = { 0 };
+    char buffer2[3] = { 0 };
+
+    printk("In %s()\n", __func__);
+    printk("Some symbols: ''%ps''
''%pS''\n", _p(printk), _p(printk));
+    printk("More symbols: ''%ps''
''%pS''\n", _p(test_printk), _p(test_printk));
+    printk("Invalid: ''%ps''
''%pS''\n", _p(0x20), _p(0x20));
+    printk("Partial: ''%ps''
''%pS''\n", _p(((unsigned long)test_printk + 0x20)),
+           _p(((unsigned long)test_printk + 0x20)));
+
+    snprintf(buffer, sizeof buffer, "%pS", _p(test_printk));
+    printk("Test printing into 8 byte buffer
''%s''\n", buffer);
+
+    snprintf(buffer, sizeof buffer, "%ps", _p(test_printk));
+    printk("Test printing into 8 byte buffer
''%s''\n", buffer);
+
+    snprintf(buffer2, sizeof buffer2, "%ps",
+             _p(((unsigned long)test_printk + 0x20)));
+    printk("Test printing into 3 byte buffer
''%s''\n", buffer2);
+}
+
+static struct keyhandler test_printk_keyhandler = {
+    .diagnostic = 1,
+    .u.fn = test_printk,
+    .desc = "Test new printk formatting options"
+};
+
+static int __init setup_test_printk(void)
+{
+    register_keyhandler(''6'', &test_printk_keyhandler);
+    return 0;
+}
+__initcall(setup_test_printk);
-- 
1.7.10.4

Jan Beulich

2013-Nov-05 10:21 UTC

head link

Re: [PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> No fuctional change
functional
> +    str = number(str, end, val, 16, field_width, precision, flags);
> +
> +    return str;
Why not simply "return number(...);"?
> @@ -399,13 +414,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
>              continue;
>  
>          case ''p'':
> -            if (field_width == -1) {
> -                field_width = 2*sizeof(void *);
> -                flags |= ZEROPAD;
> -            }
> -            str = number(str, end,
> -                         (unsigned long) va_arg(args, void *),
> -                         16, field_width, precision, flags);
> +            str = pointer(str, end, (unsigned long) va_arg(args, void *),
> +                          field_width, precision, flags);
The va_arg() result clearly shouldn''t be cast here, even if both
the use here and that added by the next patch require this. I''m
definitely planning on adding a modifier to print domain/vcpu IDs
as a pair from a single "struct vcpu *" argument, and such code
shouldn''t be required to cast back from "unsigned long" to a
pointer.

Jan

Andrew Cooper

2013-Nov-05 10:25 UTC

head link

Re: [PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

On 05/11/13 10:21, Jan Beulich wrote:>>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> No fuctional change
> functional
>
>> +    str = number(str, end, val, 16, field_width, precision, flags);
>> +
>> +    return str;
> Why not simply "return number(...);"?
I thought I did, although that appears to only have made it into the
subsequent patch.
>
>> @@ -399,13 +414,8 @@ int vsnprintf(char *buf, size_t size, const char
*fmt, va_list args)
>>              continue;
>>  
>>          case ''p'':
>> -            if (field_width == -1) {
>> -                field_width = 2*sizeof(void *);
>> -                flags |= ZEROPAD;
>> -            }
>> -            str = number(str, end,
>> -                         (unsigned long) va_arg(args, void *),
>> -                         16, field_width, precision, flags);
>> +            str = pointer(str, end, (unsigned long) va_arg(args, void
*),
>> +                          field_width, precision, flags);
> The va_arg() result clearly shouldn''t be cast here, even if both
> the use here and that added by the next patch require this. I''m
> definitely planning on adding a modifier to print domain/vcpu IDs
> as a pair from a single "struct vcpu *" argument, and such code
> shouldn''t be required to cast back from "unsigned long"
to a
> pointer.
>
> Jan
Ok - I will use a void* instead.

~Andrew
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Ian Campbell

2013-Nov-05 10:39 UTC

head link

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

On Mon, 2013-11-04 at 21:30 +0000, Andrew Cooper wrote:> +All parameters to a %p option should be enclosed with the _p() macro
For numerical arguments, sure. Why for things that are already pointers?
> +Symbol/Function pointers:
> +
> +       %ps     Symbol name only
> +       %pS     Symbol name + offset / function length
Perhaps give examples?
> +       In the case that an appropriate symbol name can''t be
found, %p[sS] will
> +       fall back to ''%p'' and print the address in hex.
In the common case the hex value will already be printed previously, but
I suppose this is harmless.

Ian.

Jan Beulich

2013-Nov-05 10:40 UTC

head link

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> -static char *pointer(char *str, char *end,
> +static char *pointer(char *str, char *end, const char **fmt_ptr,
>                       unsigned long val, int field_width, int precision,
>                       int flags)
>  {
> -    if ( field_width == -1 )
> +    const char *fmt = *fmt_ptr, *s;
> +
> +    /*
> +     * Custom %p suffixes, compatible with Linux.
> +     * See XEN_ROOT/docs/misc/printk-formats.txt
> +     */
> +    switch ( fmt[1] )
>      {
> -        field_width = 2 * sizeof(void *);
> -        flags |= ZEROPAD;
> +    case ''s'': /* Symbol name */
> +    case ''S'': /* Symbol name with offset and size */
> +    {
> +        unsigned long sym_size, sym_offset;
> +        char namebuf[KSYM_NAME_LEN+1];
> +
> +        /* Advance fmt pointer, as we have consumed ''s''
or ''S''.  Leave fmt
> +         * along for consistency. */
I''m not sure what the second sentence is supposed to tell me, even
after s/along/alone/. Also - coding style (multi-line comment), no
matter that other coding style aspects are being done matching the
rest of the file rather than ./CODING_STYLE.
> +        (*fmt_ptr)++;
++*fmt_ptr allows avoiding the parentheses.
> +
> +        s = symbols_lookup(val, &sym_size, &sym_offset, namebuf);
> +
> +        /* If the symbol is not found, fall back to printing the address
*/
> +        if ( !s )
> +            goto regular_pointer;
Rather than using "goto" here, I''d suggest using
"break" and
handling the "normal" pointer case outside of the switch.

Also I''m inclined to suggest that plain symbols be printed as
symbols only when sym_offset is zero (otherwise confusion arises
as to whether a pointer was an exact match).
> +
> +        /* Print symbol name */
> +        str = string(str, end, s, -1, -1, 0);
> +
> +        if ( fmt[1] == ''S'' )
> +        {
> +            /* Print ''+0x<offset>/0x<len>''
*/
> +            str = string(str, end, "+0x", -1, -1, 0);
> +            str = number(str, end, sym_offset, 16, -1, -1, 0);
I''d further suggest to pass SPECIAL here and avoid explicitly
printing 0x (thus allowing at least zero to be printed as plain
"0").

Similarly passing SIGN|PLUS here would allow you to drop
the explicit issuing of "+".
> +            str = string(str, end, "/0x", -1, -1, 0);
> +            str = number(str, end, sym_size, 16, -1, -1, 0);
Same here then, even though sym_size should rarely end up
being zero.

Jan

Andrew Cooper

2013-Nov-05 10:43 UTC

head link

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

On 05/11/13 10:39, Ian Campbell wrote:> On Mon, 2013-11-04 at 21:30 +0000, Andrew Cooper wrote:
>> +All parameters to a %p option should be enclosed with the _p() macro
> For numerical arguments, sure. Why for things that are already pointers?
GCC seemed funny about implicitly casting pointers to void* as far as
formatting went.  I will verify what the behaviour is on something modern.
>
>> +Symbol/Function pointers:
>> +
>> +       %ps     Symbol name only
>> +       %pS     Symbol name + offset / function length
> Perhaps give examples?
ok
>
>> +       In the case that an appropriate symbol name can''t be
found, %p[sS] will
>> +       fall back to ''%p'' and print the address in
hex.
> In the common case the hex value will already be printed previously, but
> I suppose this is harmless.
>
> Ian.
>
print_symbol() would result in "???" if it failed to get a name.  Now,
we fall through to printing the hex address instead.

~Andrew

Jan Beulich

2013-Nov-05 10:47 UTC

head link

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1475,8 +1475,7 @@ 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);
> +                printk("hpet_broadcast_setup() failed, turning to PIT
broadcast\n");
You''re losing information here - hpet_broadcast_setup is a pointer
to a function.
> --- a/xen/common/timer.c
> +++ b/xen/common/timer.c
> @@ -511,9 +511,9 @@ s_time_t align_timer(s_time_t firsttick, uint64_t
period)
>  
>  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);
> +    printk("  ex=%8"PRId64"us timer=%p cb=%p(%p)
%ps()\n",
> +           (t->expires - now) / 1000, t, t->function, t->data,
> +           _p(t->function));
Do you really think printing t->function as raw _and_ as symbolic
is worthwhile?

Also, this isn''t really x86 code...

Jan

Andrew Cooper

2013-Nov-05 10:52 UTC

head link

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

On 05/11/13 10:47, Jan Beulich wrote:>>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1475,8 +1475,7 @@ 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);
>> +                printk("hpet_broadcast_setup() failed, turning to
PIT broadcast\n");
> You''re losing information here - hpet_broadcast_setup is a pointer
> to a function.
So I am.
>
>> --- a/xen/common/timer.c
>> +++ b/xen/common/timer.c
>> @@ -511,9 +511,9 @@ s_time_t align_timer(s_time_t firsttick, uint64_t
period)
>>  
>>  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);
>> +    printk("  ex=%8"PRId64"us timer=%p cb=%p(%p)
%ps()\n",
>> +           (t->expires - now) / 1000, t, t->function,
t->data,
>> +           _p(t->function));
> Do you really think printing t->function as raw _and_ as symbolic
> is worthwhile?
Probably not, especially given the new fallback behaviour upon failing
to find a symbol name
>
> Also, this isn''t really x86 code...
>
> Jan
>
So it isn''t.  I shall split it into a patch on its own.

~Andrew

Andrew Cooper

2013-Nov-05 10:57 UTC

head link

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

On 05/11/13 10:40, Jan Beulich wrote:>>>> On 04.11.13 at 22:30, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> -static char *pointer(char *str, char *end,
>> +static char *pointer(char *str, char *end, const char **fmt_ptr,
>>                       unsigned long val, int field_width, int
precision,
>>                       int flags)
>>  {
>> -    if ( field_width == -1 )
>> +    const char *fmt = *fmt_ptr, *s;
>> +
>> +    /*
>> +     * Custom %p suffixes, compatible with Linux.
>> +     * See XEN_ROOT/docs/misc/printk-formats.txt
>> +     */
>> +    switch ( fmt[1] )
>>      {
>> -        field_width = 2 * sizeof(void *);
>> -        flags |= ZEROPAD;
>> +    case ''s'': /* Symbol name */
>> +    case ''S'': /* Symbol name with offset and size */
>> +    {
>> +        unsigned long sym_size, sym_offset;
>> +        char namebuf[KSYM_NAME_LEN+1];
>> +
>> +        /* Advance fmt pointer, as we have consumed
''s'' or ''S''.  Leave fmt
>> +         * along for consistency. */
> I''m not sure what the second sentence is supposed to tell me, even
> after s/along/alone/. Also - coding style (multi-line comment), no
> matter that other coding style aspects are being done matching the
> rest of the file rather than ./CODING_STYLE.
It was left over from a previous iteration which I thought I had
purged.  It has been now.
>
>> +        (*fmt_ptr)++;
> ++*fmt_ptr allows avoiding the parentheses.
>
>> +
>> +        s = symbols_lookup(val, &sym_size, &sym_offset,
namebuf);
>> +
>> +        /* If the symbol is not found, fall back to printing the
address */
>> +        if ( !s )
>> +            goto regular_pointer;
> Rather than using "goto" here, I''d suggest using
"break" and
> handling the "normal" pointer case outside of the switch.
>
> Also I''m inclined to suggest that plain symbols be printed as
> symbols only when sym_offset is zero (otherwise confusion arises
> as to whether a pointer was an exact match).
>
>> +
>> +        /* Print symbol name */
>> +        str = string(str, end, s, -1, -1, 0);
>> +
>> +        if ( fmt[1] == ''S'' )
>> +        {
>> +            /* Print
''+0x<offset>/0x<len>'' */
>> +            str = string(str, end, "+0x", -1, -1, 0);
>> +            str = number(str, end, sym_offset, 16, -1, -1, 0);
> I''d further suggest to pass SPECIAL here and avoid explicitly
> printing 0x (thus allowing at least zero to be printed as plain
> "0").
>
> Similarly passing SIGN|PLUS here would allow you to drop
> the explicit issuing of "+".
>
>> +            str = string(str, end, "/0x", -1, -1, 0);
>> +            str = number(str, end, sym_size, 16, -1, -1, 0);
> Same here then, even though sym_size should rarely end up
> being zero.
>
> Jan
>
Ok for all other points.

~Andrew

Jan Beulich

2013-Nov-05 11:14 UTC

head link

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

>>> On 05.11.13 at 11:52, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 05/11/13 10:47, Jan Beulich wrote:
>> Also, this isn''t really x86 code...
> 
> So it isn''t.  I shall split it into a patch on its own.
Or perhaps fold it into the patch removing print_symbol(), which
touches common code anyway.

Jan

Andrew Cooper

2013-Nov-05 11:16 UTC

head link

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

On 05/11/13 11:14, Jan Beulich wrote:>>>> On 05.11.13 at 11:52, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>> On 05/11/13 10:47, Jan Beulich wrote:
>>> Also, this isn''t really x86 code...
>> So it isn''t.  I shall split it into a patch on its own.
> Or perhaps fold it into the patch removing print_symbol(), which
> touches common code anyway.
>
> Jan
>
Ok - that works better.

~Andrew

Xen devel - Nov 2013 - [PATCH v2 0/7] Printk symbol specifier

[PATCH v2 0/7] Printk symbol specifier

[PATCH 1/7] common/vsprintf: Refactor string() out of vsnprintf()

[PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

[PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

[PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

[PATCH 5/7] arm: Replace print_symbol() with new %ps/%pS format

[PATCH 6/7] common/symbols: Remove print_symbol() and associated infrastructure

[PATCH 7/7] Test harness for new printk formatting

Re: [PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

Re: [PATCH 2/7] common/vsprintf: Refactor pointer() out of vsnprintf()

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

Re: [PATCH 3/7] common/vsprintf: Add %ps and %pS format specifier support

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format

Re: [PATCH 4/7] x86: Replace print_symbol() with new %ps/%pS format