linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] vsprintf: introduce %pT format specifier
@ 2011-03-23 12:46 Namhyung Kim
  2011-03-23 12:46 ` [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace Namhyung Kim
  2011-03-23 13:12 ` [PATCH 1/2] vsprintf: introduce %pT format specifier Ingo Molnar
  0 siblings, 2 replies; 8+ messages in thread
From: Namhyung Kim @ 2011-03-23 12:46 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Frederic Weisbecker, Steven Rostedt, linux-kernel, linux-arch

The %pT format specifier is for stack backtrace. Its handler
sprint_trace() does symbol lookup using (address-1) to ensure
the address will not point outside of the function.

If there is a tail-call to the function marked "noreturn",
gcc optimized out the code after the call then causes saved
return address points outside of the function (i.e. the start
of the next function), so pollutes call trace somewhat.
This patch will fix it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: linux-arch@vger.kernel.org
---
 include/linux/kallsyms.h |    7 +++++++
 kernel/kallsyms.c        |   40 ++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c           |    7 ++++++-
 3 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index d8e9b3d1c23c..4964ed2021cc 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -36,6 +36,7 @@ const char *kallsyms_lookup(unsigned long addr,
 
 /* Look up a kernel symbol and return it in a text buffer. */
 extern int sprint_symbol(char *buffer, unsigned long address);
+extern int sprint_trace(char *buffer, unsigned long address);
 
 /* Look up a kernel symbol and print it to the kernel messages. */
 extern void __print_symbol(const char *fmt, unsigned long address);
@@ -79,6 +80,12 @@ static inline int sprint_symbol(char *buffer, unsigned long addr)
 	return 0;
 }
 
+static inline int sprint_trace(char *buffer, unsigned long addr)
+{
+	*buffer = '\0';
+	return 0;
+}
+
 static inline int lookup_symbol_name(unsigned long addr, char *symname)
 {
 	return -ERANGE;
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091b5757..1e889477ff618 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -368,6 +368,46 @@ int sprint_symbol(char *buffer, unsigned long address)
 }
 EXPORT_SYMBOL_GPL(sprint_symbol);
 
+/**
+ * sprint_trace - Look up a kernel trace symbol and return it in a text buffer
+ * @buffer: buffer to be stored
+ * @address: address to lookup
+ *
+ * This function is for stack trace and does the same thing as sprint_symbol()
+ * but with modified/decreased @address. If there is a tail-call to the
+ * function marked "noreturn", gcc optimized out code after the call so that
+ * the stack-saved return address could point outside of the caller. This
+ * function ensures that kallsyms will find the original caller by decreasing
+ * @address and then adjusts the result by increasing offset.
+ *
+ * This function returns the number of bytes stored in @buffer.
+ */
+int sprint_trace(char *buffer, unsigned long address)
+{
+	char *modname;
+	const char *name;
+	unsigned long offset, size;
+	int len;
+
+	name = kallsyms_lookup(address-1, &size, &offset, &modname, buffer);
+	if (!name)
+		return sprintf(buffer, "0x%lx", address);
+
+	if (name != buffer)
+		strcpy(buffer, name);
+	len = strlen(buffer);
+	buffer += len;
+	offset++;
+
+	if (modname)
+		len += sprintf(buffer, "+%#lx/%#lx [%s]",
+						offset, size, modname);
+	else
+		len += sprintf(buffer, "+%#lx/%#lx", offset, size);
+
+	return len;
+}
+
 /* Look up a kernel symbol and print it to the kernel messages. */
 void __print_symbol(const char *fmt, unsigned long address)
 {
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d3023df8477f..c9c56a54cc42 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -574,7 +574,9 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	unsigned long value = (unsigned long) ptr;
 #ifdef CONFIG_KALLSYMS
 	char sym[KSYM_SYMBOL_LEN];
-	if (ext != 'f' && ext != 's')
+	if (ext == 'T')
+		sprint_trace(sym, value);
+	else if (ext != 'f' && ext != 's')
 		sprint_symbol(sym, value);
 	else
 		kallsyms_lookup(value, NULL, NULL, NULL, sym);
@@ -949,6 +951,7 @@ int kptr_restrict = 1;
  * - 'f' For simple symbolic function names without offset
  * - 'S' For symbolic direct pointers with offset
  * - 's' For symbolic direct pointers without offset
+ * - 'T' For backtraced symbolic direct pointers with offset
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
  * - 'M' For a 6-byte MAC address, it prints the address in the
@@ -1008,6 +1011,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		/* Fallthrough */
 	case 'S':
 	case 's':
+	case 'T':
 		return symbol_string(buf, end, ptr, spec, *fmt);
 	case 'R':
 	case 'r':
@@ -1279,6 +1283,7 @@ qualifier:
  * %ps output the name of a text symbol without offset
  * %pF output the name of a function pointer with its offset
  * %pf output the name of a function pointer without its offset
+ * %pT output the name of a trace symbol with its offset
  * %pR output the address range in a struct resource with decoded flags
  * %pr output the address range in a struct resource with raw flags
  * %pM output a 6-byte MAC address with colons
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace
  2011-03-23 12:46 [PATCH 1/2] vsprintf: introduce %pT format specifier Namhyung Kim
@ 2011-03-23 12:46 ` Namhyung Kim
  2011-03-23 14:14   ` Frederic Weisbecker
  2011-03-23 13:12 ` [PATCH 1/2] vsprintf: introduce %pT format specifier Ingo Molnar
  1 sibling, 1 reply; 8+ messages in thread
From: Namhyung Kim @ 2011-03-23 12:46 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Frederic Weisbecker, Steven Rostedt, linux-kernel,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/dumpstack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 914065620f2c..809d027de28f 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -27,7 +27,7 @@ static int die_counter;
 
 void printk_address(unsigned long address, int reliable)
 {
-	printk(" [<%p>] %s%pS\n", (void *) address,
+	printk(" [<%p>] %s%pT\n", (void *) address,
 			reliable ? "" : "? ", (void *) address);
 }
 
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] vsprintf: introduce %pT format specifier
  2011-03-23 12:46 [PATCH 1/2] vsprintf: introduce %pT format specifier Namhyung Kim
  2011-03-23 12:46 ` [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace Namhyung Kim
@ 2011-03-23 13:12 ` Ingo Molnar
  2011-03-23 13:29   ` [PATCH RESEND " Namhyung Kim
  1 sibling, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2011-03-23 13:12 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Andrew Morton, Frederic Weisbecker, Steven Rostedt, linux-kernel,
	linux-arch


* Namhyung Kim <namhyung@gmail.com> wrote:

> The %pT format specifier is for stack backtrace. Its handler
> sprint_trace() does symbol lookup using (address-1) to ensure
> the address will not point outside of the function.
> 
> If there is a tail-call to the function marked "noreturn",
> gcc optimized out the code after the call then causes saved
> return address points outside of the function (i.e. the start
> of the next function), so pollutes call trace somewhat.
> This patch will fix it.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: linux-arch@vger.kernel.org
> ---
>  include/linux/kallsyms.h |    7 +++++++
>  kernel/kallsyms.c        |   40 ++++++++++++++++++++++++++++++++++++++++
>  lib/vsprintf.c           |    7 ++++++-
>  3 files changed, 53 insertions(+), 1 deletions(-)

Looks useful. Please include before/after stack dump examples in the changelog, 
to show the difference.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH RESEND 1/2] vsprintf: introduce %pT format specifier
  2011-03-23 13:12 ` [PATCH 1/2] vsprintf: introduce %pT format specifier Ingo Molnar
@ 2011-03-23 13:29   ` Namhyung Kim
  2011-03-23 13:43     ` Steven Rostedt
  2011-03-23 14:16     ` Ingo Molnar
  0 siblings, 2 replies; 8+ messages in thread
From: Namhyung Kim @ 2011-03-23 13:29 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andrew Morton, Frederic Weisbecker, Steven Rostedt, linux-kernel,
	linux-arch

The %pT format specifier is for stack backtrace. Its handler
sprint_trace() does symbol lookup using (address-1) to ensure
the address will not point outside of the function.

If there is a tail-call to the function marked "noreturn",
gcc optimized out the code after the call then causes saved
return address points outside of the function (i.e. the start
of the next function), so pollutes call trace somewhat.
This patch will fix it.

before:
[   18.345923] Call Trace:
[   18.346001]  [<ffffffff812a8502>] panic+0x8c/0x18d
[   18.346257]  [<ffffffffa000012a>] deep01+0x0/0x38 [test_panic]  <--- bad
[   18.346347]  [<ffffffff81104666>] proc_file_write+0x73/0x8d
[   18.346432]  [<ffffffff811000b3>] proc_reg_write+0x8d/0xac
[   18.346516]  [<ffffffff810c7d32>] vfs_write+0xa1/0xc5
[   18.346603]  [<ffffffff810c7e0f>] sys_write+0x45/0x6c
[   18.346801]  [<ffffffff8f02943b>] system_call_fastpath+0x16/0x1b

after:
[   22.224483] Call Trace:
[   22.224569]  [<ffffffff812bce69>] panic+0x8c/0x18d
[   22.224848]  [<ffffffffa000012a>] panic_write+0x20/0x20 [test_panic]  <--- ok
[   22.224979]  [<ffffffff81115fab>] proc_file_write+0x73/0x8d
[   22.225089]  [<ffffffff81111a5f>] proc_reg_write+0x8d/0xac
[   22.225199]  [<ffffffff810d90ee>] vfs_write+0xa1/0xc5
[   22.225304]  [<ffffffff810d91cb>] sys_write+0x45/0x6c
[   22.225408]  [<ffffffff812c07fb>] system_call_fastpath+0x16/0x1b

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: linux-arch@vger.kernel.org
---
 include/linux/kallsyms.h |    7 +++++++
 kernel/kallsyms.c        |   40 ++++++++++++++++++++++++++++++++++++++++
 lib/vsprintf.c           |    7 ++++++-
 3 files changed, 53 insertions(+), 1 deletions(-)

diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index d8e9b3d1c23c..4964ed2021cc 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -36,6 +36,7 @@ const char *kallsyms_lookup(unsigned long addr,
 
 /* Look up a kernel symbol and return it in a text buffer. */
 extern int sprint_symbol(char *buffer, unsigned long address);
+extern int sprint_trace(char *buffer, unsigned long address);
 
 /* Look up a kernel symbol and print it to the kernel messages. */
 extern void __print_symbol(const char *fmt, unsigned long address);
@@ -79,6 +80,12 @@ static inline int sprint_symbol(char *buffer, unsigned long addr)
 	return 0;
 }
 
+static inline int sprint_trace(char *buffer, unsigned long addr)
+{
+	*buffer = '\0';
+	return 0;
+}
+
 static inline int lookup_symbol_name(unsigned long addr, char *symname)
 {
 	return -ERANGE;
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 6f6d091b5757..1e889477ff618 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -368,6 +368,46 @@ int sprint_symbol(char *buffer, unsigned long address)
 }
 EXPORT_SYMBOL_GPL(sprint_symbol);
 
+/**
+ * sprint_trace - Look up a kernel trace symbol and return it in a text buffer
+ * @buffer: buffer to be stored
+ * @address: address to lookup
+ *
+ * This function is for stack trace and does the same thing as sprint_symbol()
+ * but with modified/decreased @address. If there is a tail-call to the
+ * function marked "noreturn", gcc optimized out code after the call so that
+ * the stack-saved return address could point outside of the caller. This
+ * function ensures that kallsyms will find the original caller by decreasing
+ * @address and then adjusts the result by increasing offset.
+ *
+ * This function returns the number of bytes stored in @buffer.
+ */
+int sprint_trace(char *buffer, unsigned long address)
+{
+	char *modname;
+	const char *name;
+	unsigned long offset, size;
+	int len;
+
+	name = kallsyms_lookup(address-1, &size, &offset, &modname, buffer);
+	if (!name)
+		return sprintf(buffer, "0x%lx", address);
+
+	if (name != buffer)
+		strcpy(buffer, name);
+	len = strlen(buffer);
+	buffer += len;
+	offset++;
+
+	if (modname)
+		len += sprintf(buffer, "+%#lx/%#lx [%s]",
+						offset, size, modname);
+	else
+		len += sprintf(buffer, "+%#lx/%#lx", offset, size);
+
+	return len;
+}
+
 /* Look up a kernel symbol and print it to the kernel messages. */
 void __print_symbol(const char *fmt, unsigned long address)
 {
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d3023df8477f..c9c56a54cc42 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -574,7 +574,9 @@ char *symbol_string(char *buf, char *end, void *ptr,
 	unsigned long value = (unsigned long) ptr;
 #ifdef CONFIG_KALLSYMS
 	char sym[KSYM_SYMBOL_LEN];
-	if (ext != 'f' && ext != 's')
+	if (ext == 'T')
+		sprint_trace(sym, value);
+	else if (ext != 'f' && ext != 's')
 		sprint_symbol(sym, value);
 	else
 		kallsyms_lookup(value, NULL, NULL, NULL, sym);
@@ -949,6 +951,7 @@ int kptr_restrict = 1;
  * - 'f' For simple symbolic function names without offset
  * - 'S' For symbolic direct pointers with offset
  * - 's' For symbolic direct pointers without offset
+ * - 'T' For backtraced symbolic direct pointers with offset
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
  * - 'M' For a 6-byte MAC address, it prints the address in the
@@ -1008,6 +1011,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		/* Fallthrough */
 	case 'S':
 	case 's':
+	case 'T':
 		return symbol_string(buf, end, ptr, spec, *fmt);
 	case 'R':
 	case 'r':
@@ -1279,6 +1283,7 @@ qualifier:
  * %ps output the name of a text symbol without offset
  * %pF output the name of a function pointer with its offset
  * %pf output the name of a function pointer without its offset
+ * %pT output the name of a trace symbol with its offset
  * %pR output the address range in a struct resource with decoded flags
  * %pr output the address range in a struct resource with raw flags
  * %pM output a 6-byte MAC address with colons
-- 
1.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH RESEND 1/2] vsprintf: introduce %pT format specifier
  2011-03-23 13:29   ` [PATCH RESEND " Namhyung Kim
@ 2011-03-23 13:43     ` Steven Rostedt
  2011-03-23 13:49       ` Frederic Weisbecker
  2011-03-23 14:16     ` Ingo Molnar
  1 sibling, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2011-03-23 13:43 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Ingo Molnar, Andrew Morton, Frederic Weisbecker, linux-kernel,
	linux-arch

On Wed, 2011-03-23 at 22:29 +0900, Namhyung Kim wrote:
> The %pT format specifier is for stack backtrace. Its handler
> sprint_trace() does symbol lookup using (address-1) to ensure
> the address will not point outside of the function.
> 
> If there is a tail-call to the function marked "noreturn",
> gcc optimized out the code after the call then causes saved
> return address points outside of the function (i.e. the start
> of the next function), so pollutes call trace somewhat.
> This patch will fix it.
> 
> before:
> [   18.345923] Call Trace:
> [   18.346001]  [<ffffffff812a8502>] panic+0x8c/0x18d
> [   18.346257]  [<ffffffffa000012a>] deep01+0x0/0x38 [test_panic]  <--- bad
> [   18.346347]  [<ffffffff81104666>] proc_file_write+0x73/0x8d
> [   18.346432]  [<ffffffff811000b3>] proc_reg_write+0x8d/0xac
> [   18.346516]  [<ffffffff810c7d32>] vfs_write+0xa1/0xc5
> [   18.346603]  [<ffffffff810c7e0f>] sys_write+0x45/0x6c
> [   18.346801]  [<ffffffff8f02943b>] system_call_fastpath+0x16/0x1b
> 
> after:
> [   22.224483] Call Trace:
> [   22.224569]  [<ffffffff812bce69>] panic+0x8c/0x18d
> [   22.224848]  [<ffffffffa000012a>] panic_write+0x20/0x20 [test_panic]  <--- ok
> [   22.224979]  [<ffffffff81115fab>] proc_file_write+0x73/0x8d
> [   22.225089]  [<ffffffff81111a5f>] proc_reg_write+0x8d/0xac
> [   22.225199]  [<ffffffff810d90ee>] vfs_write+0xa1/0xc5
> [   22.225304]  [<ffffffff810d91cb>] sys_write+0x45/0x6c
> [   22.225408]  [<ffffffff812c07fb>] system_call_fastpath+0x16/0x1b

Nice

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RESEND 1/2] vsprintf: introduce %pT format specifier
  2011-03-23 13:43     ` Steven Rostedt
@ 2011-03-23 13:49       ` Frederic Weisbecker
  0 siblings, 0 replies; 8+ messages in thread
From: Frederic Weisbecker @ 2011-03-23 13:49 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Namhyung Kim, Ingo Molnar, Andrew Morton, linux-kernel, linux-arch

2011/3/23 Steven Rostedt <rostedt@goodmis.org>:
> On Wed, 2011-03-23 at 22:29 +0900, Namhyung Kim wrote:
>> The %pT format specifier is for stack backtrace. Its handler
>> sprint_trace() does symbol lookup using (address-1) to ensure
>> the address will not point outside of the function.
>>
>> If there is a tail-call to the function marked "noreturn",
>> gcc optimized out the code after the call then causes saved
>> return address points outside of the function (i.e. the start
>> of the next function), so pollutes call trace somewhat.
>> This patch will fix it.
>>
>> before:
>> [   18.345923] Call Trace:
>> [   18.346001]  [<ffffffff812a8502>] panic+0x8c/0x18d
>> [   18.346257]  [<ffffffffa000012a>] deep01+0x0/0x38 [test_panic]  <--- bad
>> [   18.346347]  [<ffffffff81104666>] proc_file_write+0x73/0x8d
>> [   18.346432]  [<ffffffff811000b3>] proc_reg_write+0x8d/0xac
>> [   18.346516]  [<ffffffff810c7d32>] vfs_write+0xa1/0xc5
>> [   18.346603]  [<ffffffff810c7e0f>] sys_write+0x45/0x6c
>> [   18.346801]  [<ffffffff8f02943b>] system_call_fastpath+0x16/0x1b
>>
>> after:
>> [   22.224483] Call Trace:
>> [   22.224569]  [<ffffffff812bce69>] panic+0x8c/0x18d
>> [   22.224848]  [<ffffffffa000012a>] panic_write+0x20/0x20 [test_panic]  <--- ok
>> [   22.224979]  [<ffffffff81115fab>] proc_file_write+0x73/0x8d
>> [   22.225089]  [<ffffffff81111a5f>] proc_reg_write+0x8d/0xac
>> [   22.225199]  [<ffffffff810d90ee>] vfs_write+0xa1/0xc5
>> [   22.225304]  [<ffffffff810d91cb>] sys_write+0x45/0x6c
>> [   22.225408]  [<ffffffff812c07fb>] system_call_fastpath+0x16/0x1b
>
> Nice
>
> Acked-by: Steven Rostedt <rostedt@goodmis.org>

Same for me:

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace
  2011-03-23 12:46 ` [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace Namhyung Kim
@ 2011-03-23 14:14   ` Frederic Weisbecker
  0 siblings, 0 replies; 8+ messages in thread
From: Frederic Weisbecker @ 2011-03-23 14:14 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Andrew Morton, Steven Rostedt, linux-kernel, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

2011/3/23 Namhyung Kim <namhyung@gmail.com>:
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org
> ---
>  arch/x86/kernel/dumpstack.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
> index 914065620f2c..809d027de28f 100644
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -27,7 +27,7 @@ static int die_counter;
>
>  void printk_address(unsigned long address, int reliable)
>  {
> -       printk(" [<%p>] %s%pS\n", (void *) address,
> +       printk(" [<%p>] %s%pT\n", (void *) address,
>                        reliable ? "" : "? ", (void *) address);
>  }
>
> --
> 1.7.4
>
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH RESEND 1/2] vsprintf: introduce %pT format specifier
  2011-03-23 13:29   ` [PATCH RESEND " Namhyung Kim
  2011-03-23 13:43     ` Steven Rostedt
@ 2011-03-23 14:16     ` Ingo Molnar
  1 sibling, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2011-03-23 14:16 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Andrew Morton, Frederic Weisbecker, Steven Rostedt, linux-kernel,
	linux-arch, Linus Torvalds


* Namhyung Kim <namhyung@gmail.com> wrote:

> The %pT format specifier is for stack backtrace. Its handler
> sprint_trace() does symbol lookup using (address-1) to ensure
> the address will not point outside of the function.
> 
> If there is a tail-call to the function marked "noreturn",
> gcc optimized out the code after the call then causes saved
> return address points outside of the function (i.e. the start
> of the next function), so pollutes call trace somewhat.
> This patch will fix it.
> 
> before:
> [   18.345923] Call Trace:
> [   18.346001]  [<ffffffff812a8502>] panic+0x8c/0x18d
> [   18.346257]  [<ffffffffa000012a>] deep01+0x0/0x38 [test_panic]  <--- bad
> [   18.346347]  [<ffffffff81104666>] proc_file_write+0x73/0x8d
> [   18.346432]  [<ffffffff811000b3>] proc_reg_write+0x8d/0xac
> [   18.346516]  [<ffffffff810c7d32>] vfs_write+0xa1/0xc5
> [   18.346603]  [<ffffffff810c7e0f>] sys_write+0x45/0x6c
> [   18.346801]  [<ffffffff8f02943b>] system_call_fastpath+0x16/0x1b
> 
> after:
> [   22.224483] Call Trace:
> [   22.224569]  [<ffffffff812bce69>] panic+0x8c/0x18d
> [   22.224848]  [<ffffffffa000012a>] panic_write+0x20/0x20 [test_panic]  <--- ok
> [   22.224979]  [<ffffffff81115fab>] proc_file_write+0x73/0x8d
> [   22.225089]  [<ffffffff81111a5f>] proc_reg_write+0x8d/0xac
> [   22.225199]  [<ffffffff810d90ee>] vfs_write+0xa1/0xc5
> [   22.225304]  [<ffffffff810d91cb>] sys_write+0x45/0x6c
> [   22.225408]  [<ffffffff812c07fb>] system_call_fastpath+0x16/0x1b

Ok, this looks really useful - we really want 100% perfect backtraces, kernel 
developers are looking at hundreds of thousands of call traces per year, so 
every little detail helps in the long run!

[ Nit: please omit the timestamp prefixes from the changelog, they add no 
  information and just clutter the git log. ]

The implementation could be done a bit cleaner:

> +/**
> + * sprint_trace - Look up a kernel trace symbol and return it in a text buffer
> + * @buffer: buffer to be stored
> + * @address: address to lookup
> + *
> + * This function is for stack trace and does the same thing as sprint_symbol()
> + * but with modified/decreased @address. If there is a tail-call to the
> + * function marked "noreturn", gcc optimized out code after the call so that
> + * the stack-saved return address could point outside of the caller. This
> + * function ensures that kallsyms will find the original caller by decreasing
> + * @address and then adjusts the result by increasing offset.
> + *
> + * This function returns the number of bytes stored in @buffer.
> + */
> +int sprint_trace(char *buffer, unsigned long address)
> +{
> +	char *modname;
> +	const char *name;
> +	unsigned long offset, size;
> +	int len;
> +
> +	name = kallsyms_lookup(address-1, &size, &offset, &modname, buffer);
> +	if (!name)
> +		return sprintf(buffer, "0x%lx", address);
> +
> +	if (name != buffer)
> +		strcpy(buffer, name);
> +	len = strlen(buffer);
> +	buffer += len;
> +	offset++;
> +
> +	if (modname)
> +		len += sprintf(buffer, "+%#lx/%#lx [%s]",
> +						offset, size, modname);

[ Nit: please do not break the line there, it makes the code less readable. 
  (ignore checkpatch in this case) ]

> +	else
> +		len += sprintf(buffer, "+%#lx/%#lx", offset, size);
> +
> +	return len;
> +}

This is really just a trivial variant of sprint_symbol() AFAICS, to make 
function return lookups more reliable. You look up address-1 then fix up the 
resulting offset.

I'd suggest to introduce __sprint_symbol() internal helper function, with a 
'symbol_offset' parameter to it.

That way sprint_symbol() can be implemented as a __sprint_symbol(..., 0) call, 
while sprint_trace() can be implemented as a __sprint_symbol(.., -1) call.

Also, while at it, please rename sprint_trace() to something better. This is 
not about tracing per se, this is about *backtraces* - and in particular this 
is about return addresses to noreturn functions pointing outside the kallsyms 
symbol of that function.

So a better name would be sprint_backtrace() or so?

> @@ -949,6 +951,7 @@ int kptr_restrict = 1;
>   * - 'f' For simple symbolic function names without offset
>   * - 'S' For symbolic direct pointers with offset
>   * - 's' For symbolic direct pointers without offset
> + * - 'T' For backtraced symbolic direct pointers with offset

[ Nit: 'B' might be a better abbreviation for 'backtrace'. ]

	Ingo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-03-23 14:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-03-23 12:46 [PATCH 1/2] vsprintf: introduce %pT format specifier Namhyung Kim
2011-03-23 12:46 ` [PATCH 2/2] x86, dumpstack: use %pT format specifier for stack trace Namhyung Kim
2011-03-23 14:14   ` Frederic Weisbecker
2011-03-23 13:12 ` [PATCH 1/2] vsprintf: introduce %pT format specifier Ingo Molnar
2011-03-23 13:29   ` [PATCH RESEND " Namhyung Kim
2011-03-23 13:43     ` Steven Rostedt
2011-03-23 13:49       ` Frederic Weisbecker
2011-03-23 14:16     ` Ingo Molnar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).