| Message ID | 20230808083020.22254-2-Kuan-Ying.Lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add GDB memory helper commands | expand |
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'):
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'):
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(-)