This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v3 3/8] Add basic Linux kernel support
- From: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>
- To: Omair Javaid <omair dot javaid at linaro dot org>
- Cc: Yao Qi <qiyaoltc at gmail dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, Yao Qi <yao dot qi at linaro dot org>, Peter Griffin <peter dot griffin at linaro dot org>, Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- Date: Wed, 10 May 2017 11:36:19 +0200
- Subject: Re: [RFC v3 3/8] Add basic Linux kernel support
- Authentication-results: sourceware.org; auth=none
- References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-4-prudo@linux.vnet.ibm.com> <86d1br8q8v.fsf@gmail.com> <20170503173626.15e61941@ThinkPad> <CANW4E-1RsgLsVdpNo5Sjrrua5PFHBJ5+EiwjGAiZi075cq7RSQ@mail.gmail.com> <20170508132204.7a733dc2@ThinkPad>
Hi Omair,
I forgot one thin on my bucket list.
* The way the module support maps module names to paths to .ko files should be
improved.
Currently I parse <solib-search-path>/modules.order (usually
<solib-search-path> = /lib/modules/$(uname -r)) to do so and load the path
relative <solib-search-path>. The problem is that for ubuntu the files
in /lib/modules/... are stripped off their debuginfo. And thus GDB
complains that it cannot load the modules symbols. The full files (including
debuginfo) can be found under /usr/lib/debug/lib/modules/$(uname -r)/ but this
directory doesn't contain the modules.order file ...
There is a simple workaround by copying modules.order to /usr/lib/debug/...,
nevertheless it would be nicer if the mapping would be more robust.
Philipp
On Mon, 8 May 2017 13:22:04 +0200
Philipp Rudo <prudo@linux.vnet.ibm.com> wrote:
> Hi Omair,
>
> On Mon, 8 May 2017 04:54:16 +0500
> Omair Javaid <omair.javaid@linaro.org> wrote:
>
> > Hi Phillip,
> >
> > Thanks for writing back. I hope you are feeling better now.
>
> Thanks. It will take some more time for me to get 100% fit again but at least
> the worst is over ...
>
> > I am trying to manage our basic live thread implementation within the
> > limits you have set out in your patches.
> >
> > However I am interested in knowing what are your plans for immediate
> > future like next couple of weeks.
> >
> > If you are not planning on making any particular design changes to the
> > current version of your patches then probably I will continue working
> > using your patches as base.
>
> My current plan is to finish off the work that has piled up during the two
> weeks I was sick. After that I will clean up my kernel stack unwinder for
> s390 so I have that finally gone (it already took way too much time).
>
> From then I don't have a fixed plan. On my bucket list there are some items
> without particular order and different impact to the interfaces. They are
>
> * Rebase to current master.
> With all the C++-yfication this will most likely lead to some minor changes.
>
> * C++-fy the target itself.
> As Yao mentioned in his mail [1] it would be better to classify
> struct lk_private to better fit the direction GDB is currently going to.
> In this process I would also get rid of some cleanups and further adept the
> new C++ features. Overall this will change some (but hopefully not
> many) interfaces. The biggest change will most likely be from function
> hooks (in struct lk_private_hooks) to virtual class methods (in lk_private).
>
> * Make LK_DECLARE_* macros more flexible.
> Currently lk_init_private_data aborts once any declared symbol cannot be
> found. This also makes the whole target unusable if e.g. the kernel is
> compiled without CONFIG_MODULES as then some symbols needed for module
> support cannot be found. My idea is to assign symbols to GDB-features e.g.
> module support and only turn off those features if a symbol could not be
> found.
>
> * Design a proper CLI (i.e. functions like dmesg etc.).
> This will be needed if we want others to actually use the feature.
> Shouldn't have any impact on you.
>
> * Implement separate thread_lists.
> Allow every target to manage its own thread_list. Heavy impact for you and
> a lot work for me...
>
> * Implement different target views.
> Allow the user to switch between different target views (e.g. linux_kernel
> and core/remote) and thus define the wanted level of abstraction. Even
> worse then the separate thread_lists...
>
> Long story short you don't have to divert away from my patches. Even if I
> start working on the separate thread_lists next it will definitely take quite
> a lot of time to implement. So no matter what you will most likely have a
> working patch before me ;)
>
> I hope I answered all your questions.
>
> Philipp
>
> [1] https://sourceware.org/ml/gdb-patches/2017-05/msg00004.html
>
> > Otherwise if you plan to make any further changes like going for a
> > separate thread list implementation for all layers of targets then i
> > can also divert away from your patches for a while untill next update
> > is posted.
> >
> > I am already diverting away from Peter's original implementation
> > because of some basic limitations pointed out during previous reviews.
> > I dont have reliable solution right now but trying to find one lets
> > see if i can manage to upgrade this current hack for live threads as
> > well.
> >
> > --
> > Omair.
> >
> > On 3 May 2017 at 20:36, Philipp Rudo <prudo@linux.vnet.ibm.com> wrote:r
> > > Hi Yao,
> > >
> > >
> > > On Tue, 02 May 2017 12:14:40 +0100
> > > Yao Qi <qiyaoltc@gmail.com> wrote:
> > >
> > >> Philipp Rudo <prudo@linux.vnet.ibm.com> writes:
> > >>
> > >> Hi Philipp,
> > >>
> > >> > +/* Initialize architecture independent private data. Must be called
> > >> > + _after_ symbol tables were initialized. */
> > >> > +
> > >> > +static void
> > >> > +lk_init_private_data ()
> > >> > +{
> > >> > + if (LK_PRIVATE->data != NULL)
> > >> > + htab_empty (LK_PRIVATE->data);
> > >> > +
> > >> > + LK_DECLARE_FIELD (task_struct, tasks);
> > >> > + LK_DECLARE_FIELD (task_struct, pid);
> > >> > + LK_DECLARE_FIELD (task_struct, tgid);
> > >> > + LK_DECLARE_FIELD (task_struct, thread_group);
> > >> > + LK_DECLARE_FIELD (task_struct, comm);
> > >> > + LK_DECLARE_FIELD (task_struct, thread);
> > >> > +
> > >> > + LK_DECLARE_FIELD (list_head, next);
> > >> > + LK_DECLARE_FIELD (list_head, prev);
> > >> > +
> > >> > + LK_DECLARE_FIELD (rq, curr);
> > >> > +
> > >> > + LK_DECLARE_FIELD (cpumask, bits);
> > >> > +
> > >> > + LK_DECLARE_ADDR (init_task);
> > >> > + LK_DECLARE_ADDR (runqueues);
> > >> > + LK_DECLARE_ADDR (__per_cpu_offset);
> > >> > + LK_DECLARE_ADDR (init_mm);
> > >> > +
> > >> > + LK_DECLARE_ADDR_ALIAS (__cpu_online_mask, cpu_online_mask); /*
> > >> > linux 4.5+ */
> > >> > + LK_DECLARE_ADDR_ALIAS (cpu_online_bits, cpu_online_mask); /*
> > >> > linux -4.4 */
> > >> > + if (LK_ADDR (cpu_online_mask) == -1)
> > >> > + error (_("Could not find address cpu_online_mask. Aborting."));
> > >> > +}
> > >> > +
> > >>
> > >> > +
> > >> > +/* Initialize linux kernel target. */
> > >> > +
> > >> > +static void
> > >> > +init_linux_kernel_ops (void)
> > >> > +{
> > >> > + struct target_ops *t;
> > >> > +
> > >> > + if (linux_kernel_ops != NULL)
> > >> > + return;
> > >> > +
> > >> > + t = XCNEW (struct target_ops);
> > >> > + t->to_shortname = "linux-kernel";
> > >> > + t->to_longname = "linux kernel support";
> > >> > + t->to_doc = "Adds support to debug the Linux kernel";
> > >> > +
> > >> > + /* set t->to_data = struct lk_private in lk_init_private. */
> > >> > +
> > >> > + t->to_open = lk_open;
> > >> > + t->to_close = lk_close;
> > >> > + t->to_detach = lk_detach;
> > >> > + t->to_fetch_registers = lk_fetch_registers;
> > >> > + t->to_update_thread_list = lk_update_thread_list;
> > >> > + t->to_pid_to_str = lk_pid_to_str;
> > >> > + t->to_thread_name = lk_thread_name;
> > >> > +
> > >> > + t->to_stratum = thread_stratum;
> > >> > + t->to_magic = OPS_MAGIC;
> > >> > +
> > >> > + linux_kernel_ops = t;
> > >> > +
> > >> > + add_target (t);
> > >> > +}
> > >> > +
> > >> > +/* Provide a prototype to silence -Wmissing-prototypes. */
> > >> > +extern initialize_file_ftype _initialize_linux_kernel;
> > >> > +
> > >> > +void
> > >> > +_initialize_linux_kernel (void)
> > >> > +{
> > >> > + init_linux_kernel_ops ();
> > >> > +
> > >> > + observer_attach_new_objfile (lk_observer_new_objfile);
> > >> > + observer_attach_inferior_created (lk_observer_inferior_created);
> > >> > +}
> > >> > diff --git a/gdb/lk-low.h b/gdb/lk-low.h
> > >> > new file mode 100644
> > >> > index 0000000..292ef97
> > >> > --- /dev/null
> > >> > +++ b/gdb/lk-low.h
> > >> > @@ -0,0 +1,310 @@
> > >> > +/* Basic Linux kernel support, architecture independent.
> > >> > +
> > >> > + 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/>. */ +
> > >> > +#ifndef __LK_LOW_H__
> > >> > +#define __LK_LOW_H__
> > >> > +
> > >> > +#include "target.h"
> > >> > +
> > >> > +extern struct target_ops *linux_kernel_ops;
> > >> > +
> > >> > +/* Copy constants defined in Linux kernel. */
> > >> > +#define LK_TASK_COMM_LEN 16
> > >> > +#define LK_BITS_PER_BYTE 8
> > >> > +
> > >> > +/* Definitions used in linux kernel target. */
> > >> > +#define LK_CPU_INVAL -1U
> > >> > +
> > >> > +/* Private data structs for this target. */
> > >> > +/* Forward declarations. */
> > >> > +struct lk_private_hooks;
> > >> > +struct lk_ptid_map;
> > >> > +
> > >> > +/* Short hand access to private data. */
> > >> > +#define LK_PRIVATE ((struct lk_private *) linux_kernel_ops->to_data)
> > >> > +#define LK_HOOK (LK_PRIVATE->hooks)
> > >> > +
> > >> > +struct lk_private
> > >>
> > >> "private" here is a little confusing. How about rename it to
> > >> "linux_kernel"?
> > >
> > > I called it "private" as it is the targets private data stored in its
> > > to_data hook. But I don't mind renaming it. Especially ...
> > >
> > >> > +{
> > >> > + /* Hashtab for needed addresses, structs and fields. */
> > >> > + htab_t data;
> > >> > +
> > >> > + /* Linked list to map between cpu number and original ptid from
> > >> > target
> > >> > + beneath. */
> > >> > + struct lk_ptid_map *old_ptid;
> > >> > +
> > >> > + /* Hooks for architecture dependent functions. */
> > >> > + struct lk_private_hooks *hooks;
> > >> > +};
> > >> > +
> > >>
> > >> Secondly, can we change it to a class and function pointers in
> > >> lk_private_hooks become virtual functions. gdbarch_lk_init_private
> > >> returns a pointer to an instance of sub-class of "linux_kernel".
> > >>
> > >> lk_init_private_data can be put the constructor of base class, to add
> > >> entries to "data", and sub-class (in each gdbarch) can add their own
> > >> specific stuff.
> > >
> > > ... when classifying the struct, which already is on my long ToDo-list.
> > > This struct is a left over from when I started working on the project
> > > shortly before gdb-7.12 was released. I didn't think that the
> > > C++-yfication would kick off that fast and started with plain C ...
> > >
> > > Thanks
> > > Philipp
> > >
> > >> > +
> > >> > +/* Functions to initialize private data. Do not use directly, use the
> > >> > + macros below instead. */
> > >> > +
> > >> > +extern struct lk_private_data *lk_init_addr (const char *name,
> > >> > + const char *alias, int
> > >> > silent); +extern struct lk_private_data *lk_init_struct (const char
> > >> > *name,
> > >> > + const char *alias, int
> > >> > silent);
> > >>
> > >> > +
> > >> > +/* Definitions for architecture dependent hooks. */
> > >> > +/* Hook to read registers from the target and supply their content
> > >> > + to the regcache. */
> > >> > +typedef void (*lk_hook_get_registers) (CORE_ADDR task,
> > >> > + struct target_ops *target,
> > >> > + struct regcache *regcache,
> > >> > + int regnum);
> > >> > +
> > >> > +/* Hook to return the per_cpu_offset of cpu CPU. Only architectures
> > >> > that
> > >> > + do not use the __per_cpu_offset array to determin the offset have
> > >> > to
> > >> > + supply this hook. */
> > >> > +typedef CORE_ADDR (*lk_hook_get_percpu_offset) (unsigned int cpu);
> > >> > +
> > >> > +/* Hook to map a running task to a logical CPU. Required if the
> > >> > target
> > >> > + beneath uses a different PID as struct rq. */
> > >> > +typedef unsigned int (*lk_hook_map_running_task_to_cpu) (struct
> > >> > thread_info *ti); +
> > >> > +struct lk_private_hooks
> > >> > +{
> > >> > + /* required */
> > >> > + lk_hook_get_registers get_registers;
> > >> > +
> > >> > + /* optional, required if __per_cpu_offset array is not used to
> > >> > determine
> > >> > + offset. */
> > >> > + lk_hook_get_percpu_offset get_percpu_offset;
> > >> > +
> > >> > + /* optional, required if the target beneath uses a different PID as
> > >> > struct
> > >> > + rq. */
> > >> > + lk_hook_map_running_task_to_cpu map_running_task_to_cpu;
> > >> > +};
> > >>
> > >
> >
>