Add lx-symbols <ko_path> command to support add specific
ko module.

Example output like below:
(gdb) lx-symbols mm/kasan/kasan_test.ko
loading @0xffff800002d30000: mm/kasan/kasan_test.ko

Signed-off-by: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
---
 scripts/gdb/linux/symbols.py | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/scripts/gdb/linux/symbols.py b/scripts/gdb/linux/symbols.py
index c8047f4441e6..5179edd1b627 100644
--- a/scripts/gdb/linux/symbols.py
+++ b/scripts/gdb/linux/symbols.py
@@ -111,11 +111,12 @@ lx-symbols command."""
         return "{textaddr} {sections}".format(
             textaddr=textaddr, sections="".join(args))
 
-    def load_module_symbols(self, module):
+    def load_module_symbols(self, module, module_file=None):
         module_name = module['name'].string()
         module_addr = str(module['mem'][constants.LX_MOD_TEXT]['base']).split()[0]
 
-        module_file = self._get_module_file(module_name)
+        if not module_file:
+            module_file = self._get_module_file(module_name)
         if not module_file and not self.module_files_updated:
             self._update_module_files()
             module_file = self._get_module_file(module_name)
@@ -138,6 +139,19 @@ lx-symbols command."""
         else:
             gdb.write("no module object found for '{0}'\n".format(module_name))
 
+    def load_ko_symbols(self, mod_path):
+        self.loaded_modules = []
+        module_list = modules.module_list()
+
+        for module in module_list:
+            module_name = module['name'].string()
+            module_pattern = ".*/{0}\.ko(?:.debug)?$".format(
+                module_name.replace("_", r"[_\-]"))
+            if re.match(module_pattern, mod_path) and os.path.exists(mod_path):
+                self.load_module_symbols(module, mod_path)
+                return
+        raise gdb.GdbError("%s is not a valid .ko\n" % mod_path)
+
     def load_all_symbols(self):
         gdb.write("loading vmlinux\n")
 
@@ -176,6 +190,11 @@ lx-symbols command."""
         self.module_files = []
         self.module_files_updated = False
 
+        argv = gdb.string_to_argv(arg)
+        if len(argv) == 1:
+            self.load_ko_symbols(argv[0])
+            return
+
         self.load_all_symbols()
 
         if hasattr(gdb, 'Breakpoint'):
-- 
2.18.0

On Tue, 2023-08-08 at 16:30 +0800, Kuan-Ying Lee wrote:
> Add lx-symbols <ko_path> command to support add specific
> ko module.

I'm not sure how this was supposed to work? It should have updated the
documentation, but more importantly, it shouldn't have broken the
documented usage of this command:

      The kernel (vmlinux) is taken from the current working directly. Modules (.ko)
      are scanned recursively, starting in the same directory. Optionally, the module
      search path can be extended by a space separated list of paths passed to the
      lx-symbols command.

Note how that talks about a "space separated list of paths" for which
clearly a single path seems like it should be accepted?

> @@ -138,6 +139,19 @@ lx-symbols command."""
>          else:
>              gdb.write("no module object found for '{0}'\n".format(module_name))
>  
> +    def load_ko_symbols(self, mod_path):
> +        self.loaded_modules = []
> +        module_list = modules.module_list()
> +
> +        for module in module_list:
> +            module_name = module['name'].string()
> +            module_pattern = ".*/{0}\.ko(?:.debug)?$".format(
> +                module_name.replace("_", r"[_\-]"))
> +            if re.match(module_pattern, mod_path) and os.path.exists(mod_path):
> +                self.load_module_symbols(module, mod_path)
> +                return
> +        raise gdb.GdbError("%s is not a valid .ko\n" % mod_path)
> +
>      def load_all_symbols(self):
>          gdb.write("loading vmlinux\n")
>  
> @@ -176,6 +190,11 @@ lx-symbols command."""
>          self.module_files = []
>          self.module_files_updated = False
>  
> +        argv = gdb.string_to_argv(arg)
> +        if len(argv) == 1:
> +            self.load_ko_symbols(argv[0])
> +            return

But this obviously breaks it, since passing a single path will go into
the if, then complain "some/folder/ is not a valid .ko" and exit.


But I'm not even sure how you intended this to work _at all_, because in
the context before this if, we have:

         self.module_paths = [os.path.abspath(os.path.expanduser(p))
                              for p in arg.split()]
         self.module_paths.append(os.getcwd())

so you first add the (file!) to the list of paths, and then try to load
the file by finding modules in the paths, and filtering by the specified
file? That seems ... very roundabout, and can even only work if the file
can be found via os.getcwd(), so you could never specify an absolute
filename?

All that seems counter to what the patch was meant to do.

I suspect that really you need to individually check "is it a file or a
directory" before handling any of this, and if it's actually a file,
don't use it as a _filter_ as you do in load_ko_symbols() now but
directly use it as is with load_module_symbols()?


@Jan, can we revert this? I came up with the following trivial fix that
makes it at least not break the original use case of passing a single-
entry directory list, but it really doesn't seem right one way or the
other.


--- a/scripts/gdb/linux/symbols.py
+++ b/scripts/gdb/linux/symbols.py
@@ -191,7 +191,7 @@ lx-symbols command."""
         self.module_files_updated = False
 
         argv = gdb.string_to_argv(arg)
-        if len(argv) == 1:
+        if len(argv) == 1 and os.path.isfile(argv[0]):
             self.load_ko_symbols(argv[0])
             return
 


johannes

On Tue, 12 Sep 2023 11:41:29 +0200 Johannes Berg <johannes@sipsolutions.net> wrote:

> On Tue, 2023-08-08 at 16:30 +0800, Kuan-Ying Lee wrote:
> > Add lx-symbols <ko_path> command to support add specific
> > ko module.
> 
> I'm not sure how this was supposed to work? It should have updated the
> documentation, but more importantly, it shouldn't have broken the
> documented usage of this command:
> 
>       The kernel (vmlinux) is taken from the current working directly. Modules (.ko)
>       are scanned recursively, starting in the same directory. Optionally, the module
>       search path can be extended by a space separated list of paths passed to the
>       lx-symbols command.
> 
> Note how that talks about a "space separated list of paths" for which
> clearly a single path seems like it should be accepted?
> 
> > @@ -138,6 +139,19 @@ lx-symbols command."""

Thanks, I queued a revert.

From: Andrew Morton <akpm@linux-foundation.org>
Subject: revert "scripts/gdb/symbols: add specific ko module load command"
Date: Tue Sep 12 09:19:10 AM PDT 2023

Revert 11f956538c07 ("scripts/gdb/symbols: add specific ko module load
command") due to breakage identified by Johannes Berg in [1].

Fixes: 11f956538c07 ("scripts/gdb/symbols: add specific ko module load command")
Reported-by: Johannes Berg <johannes@sipsolutions.net>
Closes: https://lkml.kernel.org/r/c44b748307a074d0c250002cdcfe209b8cce93c9.camel@sipsolutions.net [1]
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Chinwen Chang <chinwen.chang@mediatek.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Kieran Bingham <kbingham@kernel.org>
Cc: Kuan-Ying Lee <Kuan-Ying.Lee@mediatek.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Qun-Wei Lin <qun-wei.lin@mediatek.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 scripts/gdb/linux/symbols.py |   23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

--- a/scripts/gdb/linux/symbols.py~revert-scripts-gdb-symbols-add-specific-ko-module-load-command
+++ a/scripts/gdb/linux/symbols.py
@@ -111,12 +111,11 @@ lx-symbols command."""
         return "{textaddr} {sections}".format(
             textaddr=textaddr, sections="".join(args))
 
-    def load_module_symbols(self, module, module_file=None):
+    def load_module_symbols(self, module):
         module_name = module['name'].string()
         module_addr = str(module['mem'][constants.LX_MOD_TEXT]['base']).split()[0]
 
-        if not module_file:
-            module_file = self._get_module_file(module_name)
+        module_file = self._get_module_file(module_name)
         if not module_file and not self.module_files_updated:
             self._update_module_files()
             module_file = self._get_module_file(module_name)
@@ -139,19 +138,6 @@ lx-symbols command."""
         else:
             gdb.write("no module object found for '{0}'\n".format(module_name))
 
-    def load_ko_symbols(self, mod_path):
-        self.loaded_modules = []
-        module_list = modules.module_list()
-
-        for module in module_list:
-            module_name = module['name'].string()
-            module_pattern = ".*/{0}\.ko(?:.debug)?$".format(
-                module_name.replace("_", r"[_\-]"))
-            if re.match(module_pattern, mod_path) and os.path.exists(mod_path):
-                self.load_module_symbols(module, mod_path)
-                return
-        raise gdb.GdbError("%s is not a valid .ko\n" % mod_path)
-
     def load_all_symbols(self):
         gdb.write("loading vmlinux\n")
 
@@ -190,11 +176,6 @@ lx-symbols command."""
         self.module_files = []
         self.module_files_updated = False
 
-        argv = gdb.string_to_argv(arg)
-        if len(argv) == 1:
-            self.load_ko_symbols(argv[0])
-            return
-
         self.load_all_symbols()
 
         if hasattr(gdb, 'Breakpoint'):
_