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

[Omair Javaid <omair dot javaid at linaro dot org>]

Hi Phillip,

Thanks for writing back. I hope you are feeling better now.

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.

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