[PATCH] scripts/faddr2line: show the code context

[PATCH] scripts/faddr2line: show the code context

[changbin.du]

From: Changbin Du <changbin.du@intel.com>

Inspired by gdb command 'list', show the code context of target lines.
Here is a example:

$ scripts/faddr2line vmlinux native_write_msr+0x6
native_write_msr+0x6/0x20:
arch_static_branch at arch/x86/include/asm/msr.h:105
100             return EAX_EDX_VAL(val, low, high);
101     }
102
103     static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
104     {
105             asm volatile("1: wrmsr\n"
106                          "2:\n"
107                          _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
108                          : : "c" (msr), "a"(low), "d" (high) : "memory");
109     }
110
(inlined by) static_key_false at include/linux/jump_label.h:142
137     #define JUMP_TYPE_LINKED        2UL
138     #define JUMP_TYPE_MASK          3UL
139
140     static __always_inline bool static_key_false(struct static_key *key)
141     {
142             return arch_static_branch(key, false);
143     }
144
145     static __always_inline bool static_key_true(struct static_key *key)
146     {
147             return !arch_static_branch(key, true);
(inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
145     static inline void notrace
146     native_write_msr(unsigned int msr, u32 low, u32 high)
147     {
148             __wrmsr(msr, low, high);
149
150             if (msr_tracepoint_active(__tracepoint_write_msr))
151                     do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
152     }
153
154     /* Can be uninlined because referenced by paravirt */
155     static inline int notrace

Signed-off-by: Changbin Du <changbin.du@intel.com>
---
 scripts/faddr2line | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/scripts/faddr2line b/scripts/faddr2line
index 7721d5b..9e5735a 100755
--- a/scripts/faddr2line
+++ b/scripts/faddr2line
@@ -163,7 +163,17 @@ __faddr2line() {
 
 		# pass real address to addr2line
 		echo "$func+$offset/$sym_size:"
-		${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;"
+		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
+		[[ -z $file_lines ]] && return
+
+		# show each line with context
+		echo "$file_lines" | while read -r line
+		do
+			echo $line
+			eval $(echo $line | awk -F "[ :]" '{printf("n1=%d;n2=%d;f=%s",$NF-5, $NF+5, $(NF-1))}')
+			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
+		done
+
 		DONE=1
 
 	done < <(${NM} -n $objfile | awk -v fn=$func -v end=$file_end '$3 == fn { found=1; line=$0; start=$1; next } found == 1 { found=0; print line, "0x"$1 } END {if (found == 1) print line, end; }')
-- 
2.7.4

Re: [PATCH] scripts/faddr2line: show the code context

[Peter Zijlstra]

On Mon, Mar 19, 2018 at 03:23:25PM +0800, changbin.du@intel.com wrote:
> From: Changbin Du <changbin.du@intel.com>
> 
> Inspired by gdb command 'list', show the code context of target lines.
> Here is a example:
> 
> $ scripts/faddr2line vmlinux native_write_msr+0x6
> native_write_msr+0x6/0x20:
> arch_static_branch at arch/x86/include/asm/msr.h:105
> 100             return EAX_EDX_VAL(val, low, high);
> 101     }
> 102
> 103     static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
> 104     {
> 105             asm volatile("1: wrmsr\n"
> 106                          "2:\n"
> 107                          _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
> 108                          : : "c" (msr), "a"(low), "d" (high) : "memory");
> 109     }
> 110
> (inlined by) static_key_false at include/linux/jump_label.h:142
> 137     #define JUMP_TYPE_LINKED        2UL
> 138     #define JUMP_TYPE_MASK          3UL
> 139
> 140     static __always_inline bool static_key_false(struct static_key *key)
> 141     {
> 142             return arch_static_branch(key, false);
> 143     }
> 144
> 145     static __always_inline bool static_key_true(struct static_key *key)
> 146     {
> 147             return !arch_static_branch(key, true);
> (inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
> 145     static inline void notrace
> 146     native_write_msr(unsigned int msr, u32 low, u32 high)
> 147     {
> 148             __wrmsr(msr, low, high);
> 149
> 150             if (msr_tracepoint_active(__tracepoint_write_msr))
> 151                     do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> 152     }
> 153
> 154     /* Can be uninlined because referenced by paravirt */
> 155     static inline int notrace

Not a fan of this :-/ And you didn't even make it optional. Nor did you
Cc the original author of the tool.

Re: [PATCH] scripts/faddr2line: show the code context

[Peter Zijlstra]

On Tue, May 29, 2018 at 06:03:32PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 19, 2018 at 03:23:25PM +0800, changbin.du@intel.com wrote:
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > Inspired by gdb command 'list', show the code context of target lines.
> > Here is a example:
> > 
> > $ scripts/faddr2line vmlinux native_write_msr+0x6
> > native_write_msr+0x6/0x20:
> > arch_static_branch at arch/x86/include/asm/msr.h:105
> > 100             return EAX_EDX_VAL(val, low, high);
> > 101     }
> > 102
> > 103     static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
> > 104     {
> > 105             asm volatile("1: wrmsr\n"
> > 106                          "2:\n"
> > 107                          _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
> > 108                          : : "c" (msr), "a"(low), "d" (high) : "memory");
> > 109     }
> > 110
> > (inlined by) static_key_false at include/linux/jump_label.h:142
> > 137     #define JUMP_TYPE_LINKED        2UL
> > 138     #define JUMP_TYPE_MASK          3UL
> > 139
> > 140     static __always_inline bool static_key_false(struct static_key *key)
> > 141     {
> > 142             return arch_static_branch(key, false);
> > 143     }
> > 144
> > 145     static __always_inline bool static_key_true(struct static_key *key)
> > 146     {
> > 147             return !arch_static_branch(key, true);
> > (inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
> > 145     static inline void notrace
> > 146     native_write_msr(unsigned int msr, u32 low, u32 high)
> > 147     {
> > 148             __wrmsr(msr, low, high);
> > 149
> > 150             if (msr_tracepoint_active(__tracepoint_write_msr))
> > 151                     do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> > 152     }
> > 153
> > 154     /* Can be uninlined because referenced by paravirt */
> > 155     static inline int notrace
> 
> Not a fan of this :-/ And you didn't even make it optional. Nor did you
> Cc the original author of the tool.

Something like so fixes it up and attempts to make the --list version
more readable.

---
 scripts/faddr2line | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/scripts/faddr2line b/scripts/faddr2line
index 1876a741087c..a0149db00be7 100755
--- a/scripts/faddr2line
+++ b/scripts/faddr2line
@@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
 command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
 
 usage() {
-	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
+	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
 	exit 1
 }
 
@@ -166,15 +166,25 @@ __faddr2line() {
 		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
 		[[ -z $file_lines ]] && return
 
+		if [[ $LIST = 0 ]]; then
+			echo "$file_lines" | while read -r line
+			do
+				echo $line
+			done
+			DONE=1;
+			return
+		fi
+
 		# show each line with context
 		echo "$file_lines" | while read -r line
 		do
+			echo
 			echo $line
 			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
 			n1=$[$n-5]
 			n2=$[$n+5]
 			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
-			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
+			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
 		done
 
 		DONE=1
@@ -185,6 +195,10 @@ __faddr2line() {
 [[ $# -lt 2 ]] && usage
 
 objfile=$1
+
+LIST=0
+[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
+
 [[ ! -f $objfile ]] && die "can't find objfile $objfile"
 shift
 

Re: [PATCH] scripts/faddr2line: show the code context

[Josh Poimboeuf]

On Tue, May 29, 2018 at 06:26:36PM +0200, Peter Zijlstra wrote:
> On Tue, May 29, 2018 at 06:03:32PM +0200, Peter Zijlstra wrote:
> > On Mon, Mar 19, 2018 at 03:23:25PM +0800, changbin.du@intel.com wrote:
> > > From: Changbin Du <changbin.du@intel.com>
> > > 
> > > Inspired by gdb command 'list', show the code context of target lines.
> > > Here is a example:
> > > 
> > > $ scripts/faddr2line vmlinux native_write_msr+0x6
> > > native_write_msr+0x6/0x20:
> > > arch_static_branch at arch/x86/include/asm/msr.h:105
> > > 100             return EAX_EDX_VAL(val, low, high);
> > > 101     }
> > > 102
> > > 103     static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
> > > 104     {
> > > 105             asm volatile("1: wrmsr\n"
> > > 106                          "2:\n"
> > > 107                          _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
> > > 108                          : : "c" (msr), "a"(low), "d" (high) : "memory");
> > > 109     }
> > > 110
> > > (inlined by) static_key_false at include/linux/jump_label.h:142
> > > 137     #define JUMP_TYPE_LINKED        2UL
> > > 138     #define JUMP_TYPE_MASK          3UL
> > > 139
> > > 140     static __always_inline bool static_key_false(struct static_key *key)
> > > 141     {
> > > 142             return arch_static_branch(key, false);
> > > 143     }
> > > 144
> > > 145     static __always_inline bool static_key_true(struct static_key *key)
> > > 146     {
> > > 147             return !arch_static_branch(key, true);
> > > (inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
> > > 145     static inline void notrace
> > > 146     native_write_msr(unsigned int msr, u32 low, u32 high)
> > > 147     {
> > > 148             __wrmsr(msr, low, high);
> > > 149
> > > 150             if (msr_tracepoint_active(__tracepoint_write_msr))
> > > 151                     do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> > > 152     }
> > > 153
> > > 154     /* Can be uninlined because referenced by paravirt */
> > > 155     static inline int notrace
> > 
> > Not a fan of this :-/ And you didn't even make it optional. Nor did you
> > Cc the original author of the tool.
> 
> Something like so fixes it up and attempts to make the --list version
> more readable.
> 
> ---
>  scripts/faddr2line | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/faddr2line b/scripts/faddr2line
> index 1876a741087c..a0149db00be7 100755
> --- a/scripts/faddr2line
> +++ b/scripts/faddr2line
> @@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
>  command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
>  
>  usage() {
> -	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
> +	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
>  	exit 1
>  }
>  
> @@ -166,15 +166,25 @@ __faddr2line() {
>  		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
>  		[[ -z $file_lines ]] && return
>  
> +		if [[ $LIST = 0 ]]; then
> +			echo "$file_lines" | while read -r line
> +			do
> +				echo $line
> +			done
> +			DONE=1;
> +			return
> +		fi
> +
>  		# show each line with context
>  		echo "$file_lines" | while read -r line
>  		do
> +			echo
>  			echo $line
>  			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
>  			n1=$[$n-5]
>  			n2=$[$n+5]
>  			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
> -			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
> +			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
>  		done
>  
>  		DONE=1
> @@ -185,6 +195,10 @@ __faddr2line() {
>  [[ $# -lt 2 ]] && usage
>  
>  objfile=$1
> +
> +LIST=0
> +[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
> +
>  [[ ! -f $objfile ]] && die "can't find objfile $objfile"
>  shift

Yeah, this change really should have been an optional arg.  It hurt the
readability and compactness of the output.  The above looks good to me.

Care to send a proper patch?  If you send it to Linus he might apply it
directly as he did with my original patches.

-- 
Josh

Re: [PATCH] scripts/faddr2line: show the code context

[Peter Zijlstra]

On Tue, May 29, 2018 at 12:07:10PM -0500, Josh Poimboeuf wrote:
> Yeah, this change really should have been an optional arg.  It hurt the
> readability and compactness of the output.  The above looks good to me.
> 
> Care to send a proper patch?  If you send it to Linus he might apply it
> directly as he did with my original patches.

---
From: Peter Zijlstra (Intel) <peterz@infraded.org>

Commit 6870c0165feaa5 ("scripts/faddr2line: show the code context")
radically altered the output format of the faddr2line tool. And while
the new list output format might have merrit it broke my vim usage and
was hard to read.

Make the new format optional; using a '--list' argument and attempt to
make the output slightly easier to read by adding a little whitespace to
separate the different files and explicitly mark the line in question.

Cc: Changbin Du <changbin.du@intel.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Fixes: 6870c0165feaa5 ("scripts/faddr2line: show the code context")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 scripts/faddr2line | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/scripts/faddr2line b/scripts/faddr2line
index 1876a741087c..a0149db00be7 100755
--- a/scripts/faddr2line
+++ b/scripts/faddr2line
@@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
 command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
 
 usage() {
-	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
+	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
 	exit 1
 }
 
@@ -166,15 +166,25 @@ __faddr2line() {
 		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
 		[[ -z $file_lines ]] && return
 
+		if [[ $LIST = 0 ]]; then
+			echo "$file_lines" | while read -r line
+			do
+				echo $line
+			done
+			DONE=1;
+			return
+		fi
+
 		# show each line with context
 		echo "$file_lines" | while read -r line
 		do
+			echo
 			echo $line
 			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
 			n1=$[$n-5]
 			n2=$[$n+5]
 			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
-			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
+			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
 		done
 
 		DONE=1
@@ -185,6 +195,10 @@ __faddr2line() {
 [[ $# -lt 2 ]] && usage
 
 objfile=$1
+
+LIST=0
+[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
+
 [[ ! -f $objfile ]] && die "can't find objfile $objfile"
 shift
 

Re: [PATCH] scripts/faddr2line: show the code context

[Peter Zijlstra]

On Tue, May 29, 2018 at 07:24:30PM +0200, Peter Zijlstra wrote:
> From: Peter Zijlstra (Intel) <peterz@infraded.org>

Shees, you'd figure I could type my own email address by now..

From: Peter Zijlstra (Intel) <peterz@infradead.org>

> 
> Commit 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> radically altered the output format of the faddr2line tool. And while
> the new list output format might have merrit it broke my vim usage and
> was hard to read.
> 
> Make the new format optional; using a '--list' argument and attempt to
> make the output slightly easier to read by adding a little whitespace to
> separate the different files and explicitly mark the line in question.
> 
> Cc: Changbin Du <changbin.du@intel.com>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Fixes: 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  scripts/faddr2line | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/faddr2line b/scripts/faddr2line
> index 1876a741087c..a0149db00be7 100755
> --- a/scripts/faddr2line
> +++ b/scripts/faddr2line
> @@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
>  command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
>  
>  usage() {
> -	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
> +	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
>  	exit 1
>  }
>  
> @@ -166,15 +166,25 @@ __faddr2line() {
>  		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
>  		[[ -z $file_lines ]] && return
>  
> +		if [[ $LIST = 0 ]]; then
> +			echo "$file_lines" | while read -r line
> +			do
> +				echo $line
> +			done
> +			DONE=1;
> +			return
> +		fi
> +
>  		# show each line with context
>  		echo "$file_lines" | while read -r line
>  		do
> +			echo
>  			echo $line
>  			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
>  			n1=$[$n-5]
>  			n2=$[$n+5]
>  			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
> -			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
> +			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
>  		done
>  
>  		DONE=1
> @@ -185,6 +195,10 @@ __faddr2line() {
>  [[ $# -lt 2 ]] && usage
>  
>  objfile=$1
> +
> +LIST=0
> +[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
> +
>  [[ ! -f $objfile ]] && die "can't find objfile $objfile"
>  shift
>  

Re: [PATCH] scripts/faddr2line: show the code context

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]

On Tue, May 29 2018, Peter Zijlstra wrote:

> On Tue, May 29, 2018 at 12:07:10PM -0500, Josh Poimboeuf wrote:
>> Yeah, this change really should have been an optional arg.  It hurt the
>> readability and compactness of the output.  The above looks good to me.
>> 
>> Care to send a proper patch?  If you send it to Linus he might apply it
>> directly as he did with my original patches.
>
> ---
> From: Peter Zijlstra (Intel) <peterz@infraded.org>
>
> Commit 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> radically altered the output format of the faddr2line tool. And while
> the new list output format might have merrit it broke my vim usage and
> was hard to read.
>
> Make the new format optional; using a '--list' argument and attempt to
> make the output slightly easier to read by adding a little whitespace to
> separate the different files and explicitly mark the line in question.

Not commenting on your code but on the original patch.....
I've recently noticed that ADDR2LINE sometimes outputs
  (discriminator 2)
or similar at the end of the line.  This messes up the parsing.

I hacked it to work so I could keep debugging with

-		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
+		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed -e "s; $dir_prefix\(\./\)*; ;" -e "s/(discriminator [0-9]*)//")

but someone should probably find out exactly what sort of messages
ADDR2LINE produces, and make sure they are all parsed correctly.
(maybe that someone should be me, but not today).

Thanks,
NeilBrown


>
> Cc: Changbin Du <changbin.du@intel.com>
> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Fixes: 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  scripts/faddr2line | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/faddr2line b/scripts/faddr2line
> index 1876a741087c..a0149db00be7 100755
> --- a/scripts/faddr2line
> +++ b/scripts/faddr2line
> @@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
>  command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
>  
>  usage() {
> -	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
> +	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
>  	exit 1
>  }
>  
> @@ -166,15 +166,25 @@ __faddr2line() {
>  		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
>  		[[ -z $file_lines ]] && return
>  
> +		if [[ $LIST = 0 ]]; then
> +			echo "$file_lines" | while read -r line
> +			do
> +				echo $line
> +			done
> +			DONE=1;
> +			return
> +		fi
> +
>  		# show each line with context
>  		echo "$file_lines" | while read -r line
>  		do
> +			echo
>  			echo $line
>  			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
>  			n1=$[$n-5]
>  			n2=$[$n+5]
>  			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
> -			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
> +			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
>  		done
>  
>  		DONE=1
> @@ -185,6 +195,10 @@ __faddr2line() {
>  [[ $# -lt 2 ]] && usage
>  
>  objfile=$1
> +
> +LIST=0
> +[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
> +
>  [[ ! -f $objfile ]] && die "can't find objfile $objfile"
>  shift
>  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] scripts/faddr2line: show the code context

[Du, Changbin]

On Wed, May 30, 2018 at 08:01:48AM +1000, NeilBrown wrote:
> On Tue, May 29 2018, Peter Zijlstra wrote:
> 
> > On Tue, May 29, 2018 at 12:07:10PM -0500, Josh Poimboeuf wrote:
> >> Yeah, this change really should have been an optional arg.  It hurt the
> >> readability and compactness of the output.  The above looks good to me.
> >> 
> >> Care to send a proper patch?  If you send it to Linus he might apply it
> >> directly as he did with my original patches.
> >
> > ---
> > From: Peter Zijlstra (Intel) <peterz@infraded.org>
> >
> > Commit 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> > radically altered the output format of the faddr2line tool. And while
> > the new list output format might have merrit it broke my vim usage and
> > was hard to read.
> >
> > Make the new format optional; using a '--list' argument and attempt to
> > make the output slightly easier to read by adding a little whitespace to
> > separate the different files and explicitly mark the line in question.
> 
> Not commenting on your code but on the original patch.....
> I've recently noticed that ADDR2LINE sometimes outputs
>   (discriminator 2)
> or similar at the end of the line.  This messes up the parsing.
> 
> I hacked it to work so I could keep debugging with
> 
> -		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
> +		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed -e "s; $dir_prefix\(\./\)*; ;" -e "s/(discriminator [0-9]*)//")
> 
> but someone should probably find out exactly what sort of messages
> ADDR2LINE produces, and make sure they are all parsed correctly.
> (maybe that someone should be me, but not today).
> 
Hi, I have fixed it by commit 78eb0c6356 ("scripts/faddr2line: fix error when
addr2line output contains discriminator") and it is already in the mainline now.
Thank you!

> Thanks,
> NeilBrown
> 
> 
> >
> > Cc: Changbin Du <changbin.du@intel.com>
> > Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Fixes: 6870c0165feaa5 ("scripts/faddr2line: show the code context")
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  scripts/faddr2line | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/faddr2line b/scripts/faddr2line
> > index 1876a741087c..a0149db00be7 100755
> > --- a/scripts/faddr2line
> > +++ b/scripts/faddr2line
> > @@ -56,7 +56,7 @@ command -v ${SIZE} >/dev/null 2>&1 || die "size isn't installed"
> >  command -v ${NM} >/dev/null 2>&1 || die "nm isn't installed"
> >  
> >  usage() {
> > -	echo "usage: faddr2line <object file> <func+offset> <func+offset>..." >&2
> > +	echo "usage: faddr2line [--list] <object file> <func+offset> <func+offset>..." >&2
> >  	exit 1
> >  }
> >  
> > @@ -166,15 +166,25 @@ __faddr2line() {
> >  		local file_lines=$(${ADDR2LINE} -fpie $objfile $addr | sed "s; $dir_prefix\(\./\)*; ;")
> >  		[[ -z $file_lines ]] && return
> >  
> > +		if [[ $LIST = 0 ]]; then
> > +			echo "$file_lines" | while read -r line
> > +			do
> > +				echo $line
> > +			done
> > +			DONE=1;
> > +			return
> > +		fi
> > +
> >  		# show each line with context
> >  		echo "$file_lines" | while read -r line
> >  		do
> > +			echo
> >  			echo $line
> >  			n=$(echo $line | sed 's/.*:\([0-9]\+\).*/\1/g')
> >  			n1=$[$n-5]
> >  			n2=$[$n+5]
> >  			f=$(echo $line | sed 's/.*at \(.\+\):.*/\1/g')
> > -			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") {printf("%d\t%s\n", NR, $0)}' $f
> > +			awk 'NR>=strtonum("'$n1'") && NR<=strtonum("'$n2'") { if (NR=='$n') printf(">%d<", NR); else printf(" %d ", NR); printf("\t%s\n", $0)}' $f
> >  		done
> >  
> >  		DONE=1
> > @@ -185,6 +195,10 @@ __faddr2line() {
> >  [[ $# -lt 2 ]] && usage
> >  
> >  objfile=$1
> > +
> > +LIST=0
> > +[[ "$objfile" == "--list" ]] && LIST=1 && shift && objfile=$1
> > +
> >  [[ ! -f $objfile ]] && die "can't find objfile $objfile"
> >  shift
> >  



-- 
Thanks,
Changbin Du

Re: [PATCH] scripts/faddr2line: show the code context

[Du, Changbin]

On Tue, May 29, 2018 at 06:03:32PM +0200, Peter Zijlstra wrote:
> On Mon, Mar 19, 2018 at 03:23:25PM +0800, changbin.du@intel.com wrote:
> > From: Changbin Du <changbin.du@intel.com>
> > 
> > Inspired by gdb command 'list', show the code context of target lines.
> > Here is a example:
> > 
> > $ scripts/faddr2line vmlinux native_write_msr+0x6
> > native_write_msr+0x6/0x20:
> > arch_static_branch at arch/x86/include/asm/msr.h:105
> > 100             return EAX_EDX_VAL(val, low, high);
> > 101     }
> > 102
> > 103     static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
> > 104     {
> > 105             asm volatile("1: wrmsr\n"
> > 106                          "2:\n"
> > 107                          _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
> > 108                          : : "c" (msr), "a"(low), "d" (high) : "memory");
> > 109     }
> > 110
> > (inlined by) static_key_false at include/linux/jump_label.h:142
> > 137     #define JUMP_TYPE_LINKED        2UL
> > 138     #define JUMP_TYPE_MASK          3UL
> > 139
> > 140     static __always_inline bool static_key_false(struct static_key *key)
> > 141     {
> > 142             return arch_static_branch(key, false);
> > 143     }
> > 144
> > 145     static __always_inline bool static_key_true(struct static_key *key)
> > 146     {
> > 147             return !arch_static_branch(key, true);
> > (inlined by) native_write_msr at arch/x86/include/asm/msr.h:150
> > 145     static inline void notrace
> > 146     native_write_msr(unsigned int msr, u32 low, u32 high)
> > 147     {
> > 148             __wrmsr(msr, low, high);
> > 149
> > 150             if (msr_tracepoint_active(__tracepoint_write_msr))
> > 151                     do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
> > 152     }
> > 153
> > 154     /* Can be uninlined because referenced by paravirt */
> > 155     static inline int notrace
> 
> Not a fan of this :-/ And you didn't even make it optional. Nor did you
> Cc the original author of the tool.
Yeah, understand your compatibility concern, and thanks for your improvment.
I only added people from 'scripts/get_maintainer.pl'.


-- 
Thanks,
Changbin Du