This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v3 4/8] Add kernel module support for linux-kernel target
- From: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: gdb-patches at sourceware dot org, Yao Qi <yao dot qi at linaro dot org>, Peter Griffin <peter dot griffin at linaro dot org>, Omair Javaid <omair dot javaid at linaro dot org>, Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- Date: Wed, 3 May 2017 18:16:20 +0200
- Subject: Re: [RFC v3 4/8] Add kernel module support for linux-kernel target
- Authentication-results: sourceware.org; auth=none
- References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-5-prudo@linux.vnet.ibm.com> <868tmf8kn4.fsf@gmail.com>
Hi,
On Tue, 02 May 2017 14:15:43 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:
> Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
>
> > +/* Translate a kernel virtual address ADDR to a physical address. */
> > +
> > +CORE_ADDR
> > +lk_kvtop (CORE_ADDR addr)
>
> How about lk_kernel_vir_to_phy_addr?
I prefer kvtop. It's much shorter and (for my taste) is just as readable. But
I don't insist on keeping the name. Are there other opinions?
>
> > +{
> > + CORE_ADDR pgd = lk_read_addr (LK_ADDR (init_mm)
> > + + LK_OFFSET (mm_struct, pgd));
> > + return LK_HOOK->vtop (pgd, addr);
> > +}
> > +
> > +/* Restore current_target to TARGET. */
> > +static void
> > +restore_current_target (void *target)
> > +{
> > + current_target.beneath = (struct target_ops *) target;
> > +}
> > +
> > +/* Function for targets to_xfer_partial hook. */
> > +
> > +enum target_xfer_status
> > +lk_xfer_partial (struct target_ops *ops, enum target_object object,
> > + const char *annex, gdb_byte *readbuf,
> > + const gdb_byte *writebuf, ULONGEST offset, ULONGEST len,
> > + ULONGEST *xfered_len)
> > +{
> > + enum target_xfer_status ret_val;
> > + struct cleanup *old_chain = make_cleanup (restore_current_target,
> > ops);
>
> Use make_scoped_restore instead of make_cleanup?
Using a scoped_restore probably makes sense. Although I don't see the
advantage over old style cleanups other than having marginally shorter code ...
> > +
> > + current_target.beneath = ops->beneath;
> > +
>
> Any reasons you switch current_target.beneath temporarily?
Yes. lk_kvtop (at least for s390) reads memory if the address is not
physical. Thus reading a virtual address calls xfer_partial twice. Once for
the actual address and a second time for the data lk_kvtop needs. This can
lead to an endless recursion if there is a bug or memory corruption. Switching
to the target beneath prevents this.
> > + if (LK_HOOK->is_kvaddr (offset))
> > + offset = lk_kvtop (offset);
> > +
> > + ret_val = ops->beneath->to_xfer_partial (ops->beneath, object, annex,
> > + readbuf, writebuf, offset, len,
> > + xfered_len);
>
> Two spaces after "=".
Fixed, thanks.
> > diff --git a/gdb/lk-modules.c b/gdb/lk-modules.c
> > new file mode 100644
> > index 0000000..f3c559d
> > --- /dev/null
> > +++ b/gdb/lk-modules.c
> > @@ -0,0 +1,412 @@
> > +/* Handle Linux kernel modules as shared libraries.
> > +
> > + Copyright (C) 2016 Free Software Foundation, Inc.
> > +
> > + This file is part of GDB.
> > +
> > + This program is free software; you can redistribute it and/or modify
> > + it under the terms of the GNU General Public License as published by
> > + the Free Software Foundation; either version 3 of the License, or
> > + (at your option) any later version.
> > +
> > + This program is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + GNU General Public License for more details.
> > +
> > + You should have received a copy of the GNU General Public License
> > + along with this program. If not, see <http://www.gnu.org/licenses/>.
> > */ +
> > +#include "defs.h"
> > +
> > +#include "common/filestuff.h"
> > +#include "filenames.h"
> > +#include "gdbcmd.h"
> > +#include "gdbcore.h"
> > +#include "gdb_regex.h"
> > +#include "lk-lists.h"
> > +#include "lk-low.h"
> > +#include "lk-modules.h"
> > +#include "objfiles.h"
> > +#include "observer.h"
> > +#include "readline/readline.h"
> > +#include "solib.h"
> > +#include "solist.h"
> > +#include "utils.h"
> > +
> > +#include <unordered_map>
> > +#include <string>
> > +
> > +struct target_so_ops *lk_modules_so_ops = NULL;
> > +
> > +/* Info for single section type. */
> > +
> > +struct lm_info_sec
> > +{
> > + CORE_ADDR start;
> > + CORE_ADDR offset;
> > + unsigned int size;
> > +};
> > +
> > +/* Link map info to include in an allocated so_list entry. */
> > +
> > +struct lm_info
> > +{
> > + CORE_ADDR base;
> > + unsigned int size;
> > +
> > + struct lm_info_sec text;
> > + struct lm_info_sec init_text;
> > + struct lm_info_sec ro_data;
> > + struct lm_info_sec rw_data;
> > + struct lm_info_sec percpu;
> > +};
>
> Comments to these fields are needed.
Yes, you are right. I will fix it when I adjust to the classification of
lm_info.
> > +
> > +/* Build map between module name and path to binary file by reading file
> > + modules.order. Returns unordered_map with module name as key and its
> > + path as value. */
> > +
> > +std::unordered_map<std::string, std::string>
> > +lk_modules_build_path_map ()
> > +{
> > + std::unordered_map<std::string, std::string> umap;
> > + FILE *mod_order;
> > + struct cleanup *old_chain;
> > + char line[SO_NAME_MAX_PATH_SIZE + 1];
> > +
> > + mod_order = lk_modules_open_mod_order ();
> > + old_chain = make_cleanup_fclose (mod_order);
> > +
> > + line[SO_NAME_MAX_PATH_SIZE] = '\0';
> > + std::string search_path = lk_modules_expand_search_path ();
> > + while (fgets (line, SO_NAME_MAX_PATH_SIZE, mod_order))
> > + {
> > + /* Remove trailing newline. */
> > + line[strlen (line) - 1] = '\0';
> > +
> > + std::string name = lbasename (line);
> > +
> > + /* 3 = strlen (".ko"). */
> > + if (!endswith (name.c_str (), ".ko")
> > + || name.length () >= LK_MODULE_NAME_LEN + 3)
> > + continue;
> > +
> > + name = name.substr (0, name.length () - 3);
> > +
> > + /* Kernel modules are named after the files they are stored in with
> > + all minus '-' replaced by underscore '_'. Do the same to enable
> > + mapping. */
> > + for (size_t p = name.find('-'); p != std::string::npos;
> > + p = name.find ('-', p + 1))
> > + name[p] = '_';
> > +
> > + umap[name] = concat_path(search_path, line);
> > + }
> > +
> > + do_cleanups (old_chain);
> > + return umap;
> > +}
> > +
> > +/* Allocate and fill a copy of struct lm_info for module at address MOD.
> > */ +
> > +struct lm_info *
> > +lk_modules_read_lm_info (CORE_ADDR mod)
> > +{
> > + struct lm_info *lmi = XNEW (struct lm_info);
> > + struct cleanup *old_chain = make_cleanup (xfree, lmi);
> > +
>
> use std::unique_ptr to avoid cleanup.
>
> sdt::unique_ptr<lm_info> lmi (new lm_info ());
Yes, a unique_ptr is better. There are probably many cleanups I can get rid
off with the latest C++-yfication.
The only problem I currently have is that the C++-yfication rate is too high I
can barely follow and add new features to have a reliable kernel debugger.
Thanks for the hint anyway.
> > + if (LK_FIELD (module, module_core)) /* linux -4.4 */
> > + {
> > + lmi->base = lk_read_addr (mod + LK_OFFSET (module, module_core));
> > + lmi->size = lk_read_addr (mod + LK_OFFSET (module, core_size));
> > +
> > + lmi->text.start = lmi->base;
> > + lmi->text.offset = LK_HOOK->get_module_text_offset (mod);
> > + lmi->text.size = lk_read_uint (mod + LK_OFFSET (module,
> > core_text_size)); +
> > + lmi->ro_data.start = lmi->base + lmi->text.size;
> > + lmi->ro_data.offset = 0;
> > + lmi->ro_data.size = lk_read_uint (mod + LK_OFFSET (module,
> > + core_ro_size));
> > + }
> > + else /* linux 4.5+ */
> > + {
> > + CORE_ADDR mod_core = mod + LK_OFFSET (module, core_layout);
> > +
> > + lmi->base = lk_read_addr (mod_core
> > + + LK_OFFSET (module_layout, base));
> > + lmi->size = lk_read_uint (mod_core
> > + + LK_OFFSET (module_layout, size));
> > +
> > + lmi->text.start = lmi->base;
> > + lmi->text.offset = LK_HOOK->get_module_text_offset (mod);
> > + lmi->text.size = lk_read_uint (mod_core
> > + + LK_OFFSET (module_layout,
> > text_size)); +
> > + lmi->ro_data.start = lmi->base + lmi->text.size;
> > + lmi->ro_data.offset = 0;
> > + lmi->ro_data.size = lk_read_uint (mod_core
> > + + LK_OFFSET (module_layout,
> > ro_size));
> > + }
> > +
> > + lmi->rw_data.start = lmi->base + lmi->ro_data.size;
> > + lmi->rw_data.offset = 0;
> > + lmi->rw_data.size = lmi->size - lmi->ro_data.size;
> > +
> > + lmi->init_text.start = lk_read_addr (mod + LK_OFFSET (module, init));
> > + lmi->init_text.offset = 0;
> > +
> > + lmi->percpu.start = lk_read_addr (mod + LK_OFFSET (module, percpu));
> > + lmi->percpu.size = lk_read_uint (mod + LK_OFFSET (module, percpu_size));
> > + lmi->percpu.offset = 0;
> > +
> > + discard_cleanups (old_chain);
> > + return lmi;
> > +}
> > +
>