[RFC v3 3/8] Add basic Linux kernel support
Philipp Rudo
prudo@linux.vnet.ibm.com
Wed May 10 09:03:00 GMT 2017
Hi Peter,
On Tue, 9 May 2017 09:38:03 +0100
Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Philipp,
>
> I think as Omair mentioned previously I've moved teams in Linaro so not
> working on GDB Linux awareness directly anymore. But I'm still following
yes, Omair already mentioned and I was said to hear it. I wish you good
luck and hope you enjoy your new challenges.
> along in the background, and would love to see this feature merged.
Me too.
> On 8 May 2017 at 12:22, 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 ...
> >
>
> Good to hear.
Thanks a lot!
> >
> > > 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.
> >
>
> For dmesg and other OS helpers, don't you just want to rely on the GDB
> python
> implementations already in the kernel source?
>
> The idea being that you reduce the cross dependencies between the kernel and
> GDB by having this code live in the kernel source tree. Obviously there are
> still
> quite a few dependencies for the thread parsing and kernel modules support
> in the Linux kernel thread layer already, but having what you can in python
> still
> reduces the number of dependencies and on-going maintennce I think.
>
> Also in theory the python and kernel data structures should move in
> lockstep for
> a given release.
Yes, this is one possibility. Although I must admit that I would prefer to
have at least the "core helpers" implemented in GDB. This would guarantee a
core functionality even when you don't have the corresponding kernel sources
(when a customer sends a dump of an older distro it can sometimes be hard to
get the correct sources, especially when the distro patches the kernel...).
Furthermore an implementation within GDB can easier access GDBs internal
state. For example my dummy implementation for lsmod also shows if GDB
couldn't find debug information for a module. This also reduces code
duplication as the commands can access the infrastructure we need anyway to get
kernel awareness going. Of course this could also be achieved by extending GDBs
python interface. Finding out the "best" way to have a consistent user
interface I meant with "design" .
> >
> > * Implement separate thread_lists.
> > Allow every target to manage its own thread_list. Heavy impact for you
> > and a
> > lot work for me...
> >
>
> That would be very neat!
For me this is not only neat but the only clean solution. Otherwise it would
be just an other workaround for the global variables GDB uses.
Philipp
> >
> > * 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...
> >
>
> as would this :)
>
> regards,
>
> Peter.
>
>
> >
> > 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;
> > > >> > +};
> > > >>
> > > >
> > >
> >
> >
More information about the Gdb-patches
mailing list