[PATCH 0/3] scripts/decode_stacktrace.sh: improve error reporting and usability

[PATCH 0/3] scripts/decode_stacktrace.sh: improve error reporting and usability

[Luca Ceresoli]

This small series improves usability of scripts/decode_stacktrace.sh by
improving the usage text and correctly reporting when modules are built
without debugging symbols.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (3):
      scripts/decode_stacktrace.sh: remove find_module recursion and improve error reporting
      scripts/decode_stacktrace.sh: clarify command line
      scripts/decode_stacktrace.sh: add '-h' flag

 scripts/decode_stacktrace.sh | 49 ++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 22 deletions(-)
---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240311-decode_stacktrace-find_module-improvements-a02aee28fbaf

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH 1/3] scripts/decode_stacktrace.sh: remove find_module recursion and improve error reporting

[Luca Ceresoli]

The find_module() function can fail for two reasons:

 * the module was not found
 * the module was found but without debugging info

In both cases the user is reported the same error:

   WARNING! Modules path isn't set, but is needed to parse this symbol

This is misleading in case the modules path is set correctly.

find_module() is currently implemented as a recursive function based on
global variables in order to check up to 4 different paths. This is not
straightforward to read and even less to modify.

Besides, the debuginfod code at the beginning of find_module() is executed
identlcally every time the function is entered, i.e. up to 4 times per each
module search due to recursion.

To be able to improve error reporting, first rewrite the find_module()
function to remove recursion. The new version of the function iterates over
all the same (up to 4) paths as before and for each of them does the same
checks as before. At the end of the iteration it is now able to print an
appropriate error message, so that has been moved from the caller into
find_module().

Finally, when the module is found but without debugging info, mention the
two Kconfig variables one needs to set in order to have the needed
debugging symbols.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 scripts/decode_stacktrace.sh | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index fa5be6f57b00..7f3fb5e82707 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -88,31 +88,32 @@ find_module() {
 		fi
 	fi
 
-	if [[ "$modpath" != "" ]] ; then
-		for fn in $(find "$modpath" -name "${module//_/[-_]}.ko*") ; do
-			if ${READELF} -WS "$fn" | grep -qwF .debug_line ; then
-				echo $fn
-				return
-			fi
-		done
-		return 1
-	fi
-
-	modpath=$(dirname "$vmlinux")
-	find_module && return
-
-	if [[ $release == "" ]] ; then
+	if [ -z $release ] ; then
 		release=$(gdb -ex 'print init_uts_ns.name.release' -ex 'quit' -quiet -batch "$vmlinux" 2>/dev/null | sed -n 's/\$1 = "\(.*\)".*/\1/p')
 	fi
+	if [ -n "${release}" ] ; then
+		release_dirs="/usr/lib/debug/lib/modules/$release /lib/modules/$release"
+	fi
 
-	for dn in {/usr/lib/debug,}/lib/modules/$release ; do
-		if [ -e "$dn" ] ; then
-			modpath="$dn"
-			find_module && return
+	found_without_debug_info=false
+	for dir in "$modpath" "$(dirname "$vmlinux")" ${release_dirs}; do
+		if [ -n "${dir}" ] && [ -e "${dir}" ]; then
+			for fn in $(find "$dir" -name "${module//_/[-_]}.ko*") ; do
+				if ${READELF} -WS "$fn" | grep -qwF .debug_line ; then
+					echo $fn
+					return
+				fi
+				found_without_debug_info=true
+			done
 		fi
 	done
 
-	modpath=""
+	if [[ ${found_without_debug_info} == true ]]; then
+		echo "WARNING! No debugging info in module ${module}, rebuild with DEBUG_KERNEL and DEBUG_INFO" >&2
+	else
+		echo "WARNING! Cannot find .ko for module ${module}, please pass a valid module path" >&2
+	fi
+
 	return 1
 }
 
@@ -130,7 +131,6 @@ parse_symbol() {
 	else
 		local objfile=$(find_module)
 		if [[ $objfile == "" ]] ; then
-			echo "WARNING! Modules path isn't set, but is needed to parse this symbol" >&2
 			return
 		fi
 		if [[ $aarray_support == true ]]; then

-- 
2.34.1

Re: [PATCH 1/3] scripts/decode_stacktrace.sh: remove find_module recursion and improve error reporting

[Stephen Boyd]

Quoting Luca Ceresoli (2024-03-11 08:24:54)
> The find_module() function can fail for two reasons:
>
>  * the module was not found
>  * the module was found but without debugging info
>
> In both cases the user is reported the same error:
>
>    WARNING! Modules path isn't set, but is needed to parse this symbol
>
> This is misleading in case the modules path is set correctly.
>
> find_module() is currently implemented as a recursive function based on
> global variables in order to check up to 4 different paths. This is not
> straightforward to read and even less to modify.
>
> Besides, the debuginfod code at the beginning of find_module() is executed
> identlcally every time the function is entered, i.e. up to 4 times per each

s/identlcally/identically/

> module search due to recursion.
>
> To be able to improve error reporting, first rewrite the find_module()
> function to remove recursion. The new version of the function iterates over
> all the same (up to 4) paths as before and for each of them does the same
> checks as before. At the end of the iteration it is now able to print an
> appropriate error message, so that has been moved from the caller into
> find_module().
>
> Finally, when the module is found but without debugging info, mention the
> two Kconfig variables one needs to set in order to have the needed
> debugging symbols.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 1/3] scripts/decode_stacktrace.sh: remove find_module recursion and improve error reporting

[Luca Ceresoli]

Hello Stephen,

On Wed, 8 May 2024 17:35:53 -0400
Stephen Boyd <swboyd@chromium.org> wrote:

> Quoting Luca Ceresoli (2024-03-11 08:24:54)
> > The find_module() function can fail for two reasons:
> >
> >  * the module was not found
> >  * the module was found but without debugging info
> >
> > In both cases the user is reported the same error:
> >
> >    WARNING! Modules path isn't set, but is needed to parse this symbol
> >
> > This is misleading in case the modules path is set correctly.
> >
> > find_module() is currently implemented as a recursive function based on
> > global variables in order to check up to 4 different paths. This is not
> > straightforward to read and even less to modify.
> >
> > Besides, the debuginfod code at the beginning of find_module() is executed
> > identlcally every time the function is entered, i.e. up to 4 times per each  
> 
> s/identlcally/identically/

Well spotted!

Thanks for reviewing, v2 on its way.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 2/3] scripts/decode_stacktrace.sh: clarify command line

[Luca Ceresoli]

The syntax as expressed by usage() is not entirely correct: "<modules
path>" cannot be passed without "<base path>|auto". Additionally human
reading of this syntax can be subject to misunderstanding due the mixture
of '|' and '[]'.

Improve readability in various ways:
 * rewrite using two lines for the two allowed usages
 * add square brackets around "<vmlinux>" as it is optional when using
   debuginfod-find
 * move "<modules path>" to inside the square brackets of the 2nd
   positional parameter
 * use underscores instead of spaces in <...> strings

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 scripts/decode_stacktrace.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index 7f3fb5e82707..b56e79060e9f 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -5,7 +5,8 @@
 
 usage() {
 	echo "Usage:"
-	echo "	$0 -r <release> | <vmlinux> [<base path>|auto] [<modules path>]"
+	echo "	$0 -r <release>"
+	echo "	$0 [<vmlinux> [<base_path>|auto [<modules_path>]]]"
 }
 
 # Try to find a Rust demangler

-- 
2.34.1

Re: [PATCH 2/3] scripts/decode_stacktrace.sh: clarify command line

[Stephen Boyd]

Quoting Luca Ceresoli (2024-03-11 08:24:55)
> The syntax as expressed by usage() is not entirely correct: "<modules
> path>" cannot be passed without "<base path>|auto". Additionally human
> reading of this syntax can be subject to misunderstanding due the mixture
> of '|' and '[]'.
>
> Improve readability in various ways:
>  * rewrite using two lines for the two allowed usages
>  * add square brackets around "<vmlinux>" as it is optional when using
>    debuginfod-find
>  * move "<modules path>" to inside the square brackets of the 2nd
>    positional parameter
>  * use underscores instead of spaces in <...> strings
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH 3/3] scripts/decode_stacktrace.sh: add '-h' flag

[Luca Ceresoli]

When no parameters are passed, the usage instructions are presented only
when debuginfod-find is not found. This makes sense because with debuginfod
none of the positional parameters are needed. However it means that users
having debuginfod-find installed will have no chance of reading the usage
text without opening the file.

Many programs have a '-h' flag to get the usage, so add such a flag.
Invoking 'scripts/decode_stacktrace.sh -h' will now show the usage text and
exit.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 scripts/decode_stacktrace.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index b56e79060e9f..e8c9976062d0 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -7,6 +7,7 @@ usage() {
 	echo "Usage:"
 	echo "	$0 -r <release>"
 	echo "	$0 [<vmlinux> [<base_path>|auto [<modules_path>]]]"
+	echo "	$0 -h"
 }
 
 # Try to find a Rust demangler
@@ -32,7 +33,10 @@ fi
 READELF=${UTIL_PREFIX}readelf${UTIL_SUFFIX}
 ADDR2LINE=${UTIL_PREFIX}addr2line${UTIL_SUFFIX}
 
-if [[ $1 == "-r" ]] ; then
+if [[ $1 == "-h" ]] ; then
+	usage
+	exit 0
+elif [[ $1 == "-r" ]] ; then
 	vmlinux=""
 	basepath="auto"
 	modpath=""

-- 
2.34.1

Re: [PATCH 3/3] scripts/decode_stacktrace.sh: add '-h' flag

[Stephen Boyd]

Quoting Luca Ceresoli (2024-03-11 08:24:56)
> When no parameters are passed, the usage instructions are presented only
> when debuginfod-find is not found. This makes sense because with debuginfod
> none of the positional parameters are needed. However it means that users
> having debuginfod-find installed will have no chance of reading the usage
> text without opening the file.
>
> Many programs have a '-h' flag to get the usage, so add such a flag.
> Invoking 'scripts/decode_stacktrace.sh -h' will now show the usage text and
> exit.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 0/3] scripts/decode_stacktrace.sh: improve error reporting and usability

[Luca Ceresoli]

Hello,

On Mon, 11 Mar 2024 16:24:53 +0100
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> This small series improves usability of scripts/decode_stacktrace.sh by
> improving the usage text and correctly reporting when modules are built
> without debugging symbols.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Gently pinging about this series.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com