Re: [RFC v3 3/8] Add basic Linux kernel support

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

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;
> >> > +};  
> >>  
> >  
>