This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add Linux kernel thread runtime support.


Hi Yao,

Many thanks for your detailed review feedback :)

On Wed, 11 Jan 2017, Yao Qi wrote:

> On 16-12-22 17:31:04, Peter Griffin wrote:
> > This patch implements a new linux-kthread target_ops stratum which
> > supports the Linux kernel thread runtime. It allows anything
> > that the Linux kernel has created a task_struct for to be represented
> > as a GDB thread object. This allows a user using GDB to debug the
> > Linux kernel to see all the sleeping threads in the system rather than
> > just physical CPU's (as is the case currently) and then use the GDB
> > contextual commands such as 'thread' to easily switch between all the
> > threads in the system, inspect data structures and get backtraces etc.
> > 
> > e.g.
> > (gdb) info threads
> >   Id   Target Id         Frame
> > * 1    [swapper/0] (pid: 0 tgid: 0 <C0>) cpu_v7_do_idle ()
> >     at /sources/linux/arch/arm/mm/proc-v7.S:75
> >   2    init (pid: 1 tgid: 1) context_switch (cookie=...,
> >        next=<optimized out>, prev=<optimized out>, rq=<optimized out>)
> >     at /sources/linux/kernel/sched/core.c:2902
> >   3    [kthreadd] (pid: 2 tgid: 2) context_switch (cookie=...,
> >        next=<optimized out>, prev=<optimized out>,
> >     rq=<optimized out>) at /sources/linux/kernel/sched/core.c:2902
> > <snip>
> >   90   getty (pid: 1584 tgid: 1584) context_switch (cookie=...,
> >        next=<optimized out>, prev=<optimized out>,
> >     rq=<optimized out>) at /sources/linux/kernel/sched/core.c:2902
> >   91   udevd (pid: 1586 tgid: 1586) context_switch (cookie=...,
> >        next=<optimized out>, prev=<optimized out>,
> >     rq=<optimized out>) at /sources/linux/kernel/sched/core.c:2902
> 
> Do you have some tutorials about using this feature in GDB with QEMU
> to debug linux kernel?  I'd like to try this patch.

I have just updated and put together this tutorial / wiki page here
https://wiki.linaro.org/LandingTeams/ST/GDB.

This covers building QEMU, Linux, and binutils-gdb for ARM so you can test
in a purely virtual enviroment. This is using the build system developed
by Kieran, and I'm hoping this will form the basis for some automated testing
in the future. In fact I was recently working on adding PowerPC QEMU support
so we can also test 'virtually' on a big endian target. Obviously we can
extend for x86, arm64, x86-64 targets in the future as they are all supported
in QEMU.

> 
> > 
> > linux-kthread.ch is split between the linux-kthread target_ops stratum
> > methods themselves which are fairly self explanatory, helper functions
> > which parse kernel data structures such as CPU runqueue for idle and curr
> > tasks and a series of low level helper functions and macros which make
> > obtaining symbol information and calculating struct field offsets
> > of the various Linux kernel data structures much easier.
> 
> Looks much kernel knowledge is involved in this patch, so could you add
> some comments on the kernel data structures and how gdb parse them to
> get the list of threads?

Yes I will add more verbose comments in V2 for the kernel data structures and
why they are required.

> 
> > diff --git a/gdb/arm-linux-kthread.c b/gdb/arm-linux-kthread.c
> > new file mode 100644
> > index 0000000..a4352ac
> > --- /dev/null
> > +++ b/gdb/arm-linux-kthread.c
> > @@ -0,0 +1,178 @@
> > +/* Linux kernel thread ARM target support.
> > +
> > +   Copyright (C) 2011-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 "gdbcore.h"
> > +#include "regcache.h"
> > +#include "inferior.h"
> > +#include "arch/arm.h"
> > +#include "arm-tdep.h"
> > +#include "linux-kthread.h"
> > +#include "arm-linux-kthread.h"
> > +
> > +/* Support for Linux kernel threads */
> > +
> > +/* From Linux arm/include/asm/thread_info.h */
> > +static struct cpu_context_save
> > +{
> > +  uint32_t r4;
> > +  uint32_t r5;
> > +  uint32_t r6;
> > +  uint32_t r7;
> > +  uint32_t r8;
> > +  uint32_t r9;
> > +  uint32_t sl;
> > +  uint32_t fp;
> > +  uint32_t sp;
> > +  uint32_t pc;
> > +} cpu_cxt;
> > +
> > +/* This function gets the register values that the schedule() routine
> > + * has stored away on the stack to be able to restart a sleeping task.
> > + *
> > + **/
> 
> We don't write comments in this way.  See
> https://www.gnu.org/prep/standards/standards.html#Comments

Will fix in v2.

Is there an automated way you use for checking style issues to GDB coding
style (like checkpatch.pl in the kernel)?

> 
> > +
> > +static void
> > +arm_linuxkthread_fetch_registers (struct regcache *regcache,
> > +			 int regnum, CORE_ADDR task_struct)
> > +{
> > +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> > +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +
> > +  CORE_ADDR sp = 0;
> > +  gdb_byte buf[8];
> > +  int i;
> > +  uint32_t cpsr;
> > +  uint32_t thread_info_addr;
> > +
> > +  DECLARE_FIELD (thread_info, cpu_context);
> > +  DECLARE_FIELD (task_struct, stack);
> > +
> > +  gdb_assert (regnum >= -1);
> > +
> > +  /*get thread_info address */
> > +  thread_info_addr = read_unsigned_field (task_struct, task_struct, stack,
> > +					  byte_order);
> > +
> > +  /*get cpu_context as saved by scheduled */
> > +  read_memory ((CORE_ADDR) thread_info_addr +
> > +	       F_OFFSET (thread_info, cpu_context),
> > +	       (gdb_byte *) & cpu_cxt, sizeof (struct cpu_context_save));
> 
> You are assuming the struct cpu_context_save layout is the same on both
> target and host, however, they can be different.  The right approach, IMO,
> is to rely on the debug information to get the offset of each fields, and
> read out each field one by one.

Yes I agree, this should be taken from the debug info. Will fix in v2.
> 
> > +
> > +  regcache_raw_supply (regcache, ARM_PC_REGNUM, &cpu_cxt.pc);
> > +  regcache_raw_supply (regcache, ARM_SP_REGNUM, &cpu_cxt.sp);
> > +  regcache_raw_supply (regcache, ARM_FP_REGNUM, &cpu_cxt.fp);
> > +
> > +  /*general purpose registers */
> > +  regcache_raw_supply (regcache, 10, &cpu_cxt.sl);
> > +  regcache_raw_supply (regcache, 9, &cpu_cxt.r9);
> > +  regcache_raw_supply (regcache, 8, &cpu_cxt.r8);
> > +  regcache_raw_supply (regcache, 7, &cpu_cxt.r7);
> > +  regcache_raw_supply (regcache, 6, &cpu_cxt.r6);
> > +  regcache_raw_supply (regcache, 5, &cpu_cxt.r5);
> > +  regcache_raw_supply (regcache, 4, &cpu_cxt.r4);
> > +
> > +  /* Fake a value for cpsr:T bit.  */
> > +#define IS_THUMB_ADDR(addr)	((addr) & 1)
> > +  cpsr = IS_THUMB_ADDR(cpu_cxt.pc) ? arm_psr_thumb_bit (target_gdbarch ()) : 0;
> 
> Looks you fake the cpsr value completely.  GDB can't access cpsr value?

I will double check on this point and get back to you.
> 
> > +  regcache_raw_supply (regcache, ARM_PS_REGNUM, &cpsr);
> > +
> > +  for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++)
> > +    if (REG_VALID != regcache_register_status (regcache, i))
> > +      /* Mark other registers as unavailable.  */
> > +      regcache_invalidate (regcache, i);
> > +}
> > +
> > +static void
> > +arm_linuxkthread_store_registers (const struct regcache *regcache,
> > +			   int regnum, CORE_ADDR addr)
> > +{
> > +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> > +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +
> > +  /* TODO */
> > +  gdb_assert (regnum >= -1);
> > +  gdb_assert (0);
> 
> It is a TODO item for your patch V2.

Um, yes I guess so. Implementing the callback will enable registers
saved onto the stack to be altered. Which could be useful in certain
situations I guess.

> 
> > +
> > +}
> > +
> > +/* get_unmapped_area() in linux/mm/mmap.c.  */
> > +DECLARE_ADDR (get_unmapped_area);
> > +
> > +#define DEFAULT_PAGE_OFFSET 0xC0000000
> > +
> > +void arm_linuxkthread_get_page_offset(CORE_ADDR *page_offset)
> > +{
> > +  const char *result = NULL;
> > +
> > +  /* We can try executing a python command if it exists in the kernel
> > +      source, and parsing the result.
> > +      result = execute_command_to_string ("lx-pageoffset", 0); */
> > +
> > +  /* Find CONFIG_PAGE_OFFSET macro definition at get_unmapped_area symbol
> > +     in linux/mm/mmap.c.  */
> > +
> > +  result = kthread_find_macro_at_symbol(&get_unmapped_area,
> 
> Space is needed before "(".  Many instances around your patch.

Will fix all occurrences in v2.

> 
> > +					"CONFIG_PAGE_OFFSET");
> > +  if (result)
> > +    {
> > +      *page_offset = strtol(result, (char **) NULL, 16);
> > +    }
> > +  else
> > +    {
> > +      /* Kernel is compiled without macro info so make an educated guess.  */
> > +      warning("Assuming PAGE_OFFSET is 0x%x. Disabling to_interrupt\n",
> > +	      DEFAULT_PAGE_OFFSET);
> > +      /* PAGE_OFFSET can't be reliably determined so disable the target_ops
> > +	 to_interrupt ability. This means target can onbly be halted via
> > +	 a breakpoint set in the kernel, which will mean CPU is configured
> > +	 for kernel memory view.  */
> > +      lkthread_disable_to_interrupt = 1;
> > +      *page_offset = DEFAULT_PAGE_OFFSET;
> > +    }
> > +
> > +  return;
> > +}
> 
> This looks very fragile to me.  This function is used to determine whether
> PC is kernel space or not, and we only use this information to avoid
> interrupting the kernel when the pc is in user space.

Yes that is correct.

> Why don't you always
> disable interrupt in linux-kthread target?  That is a reasonable limitation
> to me, but the code is much clean.

We could do that, but it makes debugging a live Linux kernel target much more
difficult as all the breakpoints need to be set in advance.

If you think about commercial tools like Lauterbach, ARM DS5 etc they all
support interrupting the target, so I left this in to show one way in which
we can support this. Currently linux-kthread only disables interrupt capability
if the kernel hasn't been compiled with -g3 for pre-processor information.

Incidentally by default CONFIG_DEBUG_INFO in the kernel doesn't use -g3, so
interrupt capability will be disabled by default.

>From a personal PoV having used linux-kthread layer with the ability to
interrupt the target arbitarily (both with STMC2 jtag debugger and more recently
with QEMU / OpenOCD, disabling interrupt capability has a very noticeable impact
on the ability to debug the kernel, so my personal preference would be to find
a robust way of supporting interrupting the target at any point.

> 
> > +struct linux_kthread_data
> > +{
> > +  /* the processes list from Linux perspective */
> > +  linux_kthread_info_t *process_list = NULL;
> > +
> > +  /* the process we stopped at in target_wait */
> > +  linux_kthread_info_t *wait_process = NULL;
> > +
> > +  /* __per_cpu_offset */
> > +  CORE_ADDR *per_cpu_offset;
> > +
> > +  /* array of cur_rq(cpu) on each cpu */
> > +  CORE_ADDR *rq_curr;
> > +
> > +  /*array of rq->idle on each cpu */
> > +  CORE_ADDR *rq_idle;
> 
> It would be nice that you can explain how these three fields are used.
>

OK, will add more verbose comments in V2.

> > +
> > +  /* array of scheduled process on each core */
> > +  linux_kthread_info_t **running_process = NULL;
> 
>      std::vector<linux_kthread_info_t *> running_process;?

Will use std::vector in v2. To be honest I wasn't aware c++ types were
allowed until now.

> 
> > +
> > +  /* array of process_counts for each cpu used for process list
> > +     housekeeping */
> > +  unsigned long *process_counts;
> > +
> > +  /* Storage for the field layout and addresses already gathered. */
> > +  struct field_info *field_info_list;
> > +  struct addr_info *addr_info_list;
> > +
> > +  unsigned char *scratch_buf;
> > +  int scratch_buf_size;
> > +};
> > +
> > +/* Handle to global lkthread data.  */
> > +static struct linux_kthread_data *lkthread_h;
> > +
> > +/* Helper function to convert ptid to a string.  */
> > +
> > +static char *
> > +ptid_to_str (ptid_t ptid)
> > +{
> > +  static char str[32];
> > +  snprintf (str, sizeof (str) - 1, "ptid %d: lwp %ld: tid %ld",
> > +	    ptid_get_pid (ptid), ptid_get_lwp (ptid), ptid_get_tid (ptid));
> > +
> > +  return str;
> > +}
> > +
> > +/* Symbol and Field resolution helper functions.  */
> > +
> 
> I don't expect seeing so much code on symbol handling in linux-kthread
> patch.  linux-kthread just needs to query GDB symbol and type sub-system
> to know where a given field is in the target memory.

Maybe as other threading layers are added, some functions can be generalised
and moved out so others can take advantage of the infastructure provided.
> 
> > +/* Helper function called by ADDR macro to fetch the address of a symbol
> > +   declared using DECLARE_ADDR macro.  */
> > +
> > +int
> > +lkthread_lookup_addr (struct addr_info *addr, int check)
> > +{
> > +  if (addr->bmsym.minsym)
> > +    return 1;
> > +
> > +  addr->bmsym = lookup_minimal_symbol (addr->name, NULL, NULL);
> > +
> > +  if (!addr->bmsym.minsym)
> > +    {
> > +      if (debug_linuxkthread_symbols)
> > +	fprintf_unfiltered (gdb_stdlog, "Checking for address of '%s' :"
> > +			    "NOT FOUND\n", addr->name);
> > +
> > +      if (!check)
> > +	error ("Couldn't find address of %s", addr->name);
> > +      return 0;
> > +    }
> > +
> > +  /* Chain initialized entries for cleanup. */
> > +  addr->next = lkthread_h->addr_info_list;
> > +  lkthread_h->addr_info_list = addr;
> > +
> > +  if (debug_linuxkthread_symbols)
> > +    fprintf_unfiltered (gdb_stdlog, "%s address is %s\n", addr->name,
> > +			phex (BMSYMBOL_VALUE_ADDRESS (addr->bmsym), 4));
> > +
> > +  return 1;
> > +}
> > +
> > +/* Helper for lkthread_lookup_field.  */
> > +
> > +static int
> > +find_struct_field (struct type *type, char *field, int *offset, int *size)
> > +{
> > +  int i;
> > +
> > +  for (i = 0; i < TYPE_NFIELDS (type); ++i)
> > +    {
> > +      if (!strcmp (FIELD_NAME (TYPE_FIELDS (type)[i]), field))
> 
> use TYPE_FIELD_NAME (type, i)? which is shorter.

Fixed in v2.

> 
> 
> > +	break;
> > +    }
> > +
> > +  if (i >= TYPE_NFIELDS (type))
> > +    return 0;
> > +
> > +  *offset = FIELD_BITPOS (TYPE_FIELDS (type)[i]) / TARGET_CHAR_BIT;
> 
>      *offset = TYPE_FIELD_BITPOS (type, i) / TARGET_CHAR_BIT;
> 
> > +  *size = TYPE_LENGTH (check_typedef (TYPE_FIELDS (type)[i].type));
> > +  return 1;
> > +}
> 
> This function can be generalized so that it can be used in other parts
> of GDB.
> 
> /* Find the field by the name FIELD in TYPE.  Return the field id if
>    found, otherwise, return -1.  */
> 
> int
> type_find_field (struct *type, const char *field)
> {
>   int i;
> 
>   for (i = 0; i < TYPE_NFIELDS (type); ++i)
>   {
>     if (strcmp (TYPE_FIELD_NAME (type, i), field) == 0)
>       return i;
>   }
>   return -1;
> }
> 
> This function can be added to gdbtype.c and this function can be used in
> ada-exp.y:convert_char_literal.  You can call this function to get the
> size and offset of a given field.

Done as you suggest in V2. I will send this as a separate patch so it
can be applied before the rest of linux-kthread.

> 
> > +
> > +/* Called by F_OFFSET or F_SIZE to compute the description of a field
> > +   declared using DECLARE_FIELD.  */
> > +
> > +int
> > +lkthread_lookup_field (struct field_info *f, int check)
> > +{
> > +
> > +  if (f->type != NULL)
> > +    return 1;
> > +
> > +  f->type =
> > +    lookup_symbol (f->struct_name, NULL, STRUCT_DOMAIN, NULL).symbol;
> > +
> > +  if (!f->type)
> > +    {
> > +      f->type = lookup_symbol (f->struct_name, NULL, VAR_DOMAIN,
> > +				   NULL).symbol;
> > +
> 
> If we are looking for a struct/union, don't have to search in VAR_DOMAIN.

Will remove in v2.
> 
> > +      if (f->type && TYPE_CODE (check_typedef (SYMBOL_TYPE (f->type)))
> > +	  != TYPE_CODE_STRUCT)
> > +	f->type = NULL;
> > +
> > +    }
> > +
> > +  if (f->type == NULL
> > +      || !find_struct_field (check_typedef (SYMBOL_TYPE (f->type)),
> > +			     f->field_name, &f->offset, &f->size))
> > +    {
> > +      f->type = NULL;
> > +      if (!check)
> > +	error ("No such field %s::%s\n", f->struct_name, f->field_name);
> > +
> > +      return 0;
> > +    }
> > +
> > +  /* Chain initialized entries for cleanup. */
> > +  f->next = lkthread_h->field_info_list;
> > +  lkthread_h->field_info_list = f;
> > +
> > +  if (debug_linuxkthread_symbols)
> > +    {
> > +      fprintf_unfiltered (gdb_stdlog, "Checking for 'struct %s' : OK\n",
> > +			  f->struct_name);
> > +      fprintf_unfiltered (gdb_stdlog, "%s::%s => offset %i  size %i\n",
> > +			  f->struct_name, f->field_name, f->offset, f->size);
> > +    }
> > +  return 1;
> > +}
> > +
> > +
> 
> 
> > +
> > +/* Initialise and allocate memory for linux-kthread module.  */
> > +
> > +static void
> > +lkthread_init (void)
> > +{
> > +  struct thread_info *th = NULL;
> > +  struct cleanup *cleanup;
> > +  int size =
> > +    TYPE_LENGTH (builtin_type (target_gdbarch ())->builtin_unsigned_long);
> > +
> > +  /* Ensure thread list from beneath target is up to date.  */
> > +  cleanup = make_cleanup_restore_integer (&print_thread_events);
> > +  print_thread_events = 0;
> > +  update_thread_list ();
> > +  do_cleanups (cleanup);
> > +
> > +  /* Count the h/w threads.  */
> > +  max_cores = thread_count ();
> 
> I am confused by this line.  Could you explain?

At this point linux-kthread target_ops layer hasn't been pushed, so we are
counting the number of hw threads created by the layer beneath (gdbremote).

We assume here that this matches the physical number of CPU's on the target.
Although Philip pointed out I shouldn't do this, and should really use
cpu_online_mask, as this assumption could be incorrect.

> 
> > +  gdb_assert (max_cores);
> > +
> > +  if (debug_linuxkthread_threads)
> > +    {
> > +      fprintf_unfiltered (gdb_stdlog, "lkthread_init() cores(%d) GDB"
> > +			  "HW threads\n", max_cores);
> > +      iterate_over_threads (thread_print_info, NULL);
> > +    }
> > +
> > +  /* Allocate per cpu data.  */
> > +  lkthread_alloc_percpu_data(max_cores);
> > +
> > +  lkthread_get_per_cpu_offsets(max_cores);
> 
> Why do we need to model per_cpu data in GDB side?  I assume kernel has
> a global list of all tasks/threads, and GDB can read the element one
> by one from that list, and create a list of threads in its side.

We use the per_cpu_offsets info for a couple of things in linux-kthread
layer.

1) to get the runqueue struct addr for the CPU, which contains the rq->curr
task_struct address (which is the task_struct currently executing on the CPU).

2) to get rq->idle task_struct address (which is task_struct for the CPU's
idle / swapper task).

You can look at struct rq here
http://lxr.free-electrons.com/source/kernel/sched/sched.h#L590. This will
hopefully be clearer in V2 with more verbose comments as to why various
data structures are required.

1 & 2 are then used to give an accurate view in GDB of what threads the
physical CPU's are currently executing when the target was halted.

Essentially by matching rq->curr against rq->idle (we are in idle task) or
by matching rq->curr, with an address in the list of Linux task_structs.
This is conveyed to the user when they do a 'info threads' command by
the linux_kthread_extra_thread_info callback which appends " <C%u>" ,core
to the name. This is important, as it is more difficult to see where the
CPU's are currently executing when all the sleeping threads are shown by
GDB.

3) On SMP targets we also use it to get the process_count, and use this
to determine whether we need to re-build the thread list or not.

> 
> > diff --git a/gdb/linux-kthread.h b/gdb/linux-kthread.h
> > new file mode 100644
> > index 0000000..cffa0f4
> > --- /dev/null
> > +++ b/gdb/linux-kthread.h
> > @@ -0,0 +1,223 @@
> > +/* Linux kernel-level threads support.
> > +
> > +   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 LINUX_KTHREAD_H
> > +#define LINUX_KTHREAD_H 1
> > +
> > +#include "objfiles.h"
> > +
> > +struct addr_info
> > +{
> > +  char *name;
> > +  struct bound_minimal_symbol bmsym;
> 
> Why do you use bound_minimal_symbol instead of minimal_symbol?
> bound_minimal_symbol.objfile is not interesting here.

The only reason is that is what lookup_minimal_symbol() API returns. Looking
at ada-tasks.c and aix-thread.c they also use lookup_minimal_symbol API. Is
there another API which I should be using instead?

> 
> > +  /* Chained to allow easy cleanup.  */
> > +  struct addr_info *next;
> > +};
> 
> IIUC, each entry represent an global variable in kernel, so it is better
> to be named as "variable" or "variable_info".

Changed to variable_info in v2.

> Secondly, the struct
> addr_info don't need to be a list.  It can be an array since all the
> variables GDB wants to know in kernel is pre-determined.  We can have
> an array, std::array<struct minimal_symbol *, N>variables, and manually
> allocate index to each global variable in kernel.

Have changed it to how you describe in V2. However currently it is still an
array of bound_minimal_symbol not minimal_symbol for lookup_minimal_symbol().

The only other useful thing in addr_info struct was the name which is
useful for error reporting if things aren't found. What are your thoughts
on that?

In V2 currently I have

typedef enum
{
  init_task,
  init_pid_ns,
  __per_cpu_offset,
  per_cpu__process_counts,
  process_counts,
  per_cpu__runqueues,
  runqueues,
  variable_index_last,
} variable_index_t;

char *variable_names[] {
  "init_task",
  "init_pid_ns",
  "__per_cpu_offset",
  "per_cpu__process_counts",
  "process_counts",
  "per_cpu__runqueues",
  "runqueues"};

std::array<struct bound_minimal_symbol, variable_index_last> variables;

and access for example like

variables[index] =
    lookup_minimal_symbol (variable_names[index], NULL, NULL);
    
The name is also useful for error reporting.

> 
> > +
> > +struct field_info
> > +{
> > +  char *struct_name;
> > +  char *field_name;
> > +  struct symbol *type;
> 
> s/struct symbol/struct type/ because we need the type of the struct
> instead of the symbol.
> 
> > +  int offset;
> > +  int size;
> > +  /* Chained to allow easy cleanup.  */
> > +  struct field_info *next;
> > +};
> 
> Don't need to record much information here, we only need the type of
> the struct and its field id in the type.  struct field_info can be
> put into an array instead of list, because all the structs and fields
> GDB wants to access is pre-determined.
> 
> /* A field F in struct is represented as the struct below.  */
> 
> struct field_info
> {
>   /* The type of struct S.  */
>   struct type* s;
> 
>   /* F's field id in struct S.  */
>   int field_id;
> };
> 
> and you can allocated index for each struct and field combination.
> 
> enum field_index
> {
>   FIELD_INFO (thread_info, cpu_context),
>   FIELD_INFO (task_struct, stack),
>   FIELD_INFO (task_struct, active_mm),
>   ...
>   field_index_last,
> }
> 
> 
> std::array<struct field_info, field_index_last> fields;
> 
> and access the field_info array like this,
> 
> fields[FIELD_INFO (task_struct, stack)] = xxxx
> 
> initialize the elements in array fields to get the type and field id
> of each field.  Then, you can easily get the size and offset of
> field by TYPE and FIELD_ID.

OK, will update like you suggest in v2, and I'm hoping all those
extra fields in struct field_info really aren't required :)

Might ping you on IRC if I have a question related to this change.

> > +
> > +
> > +/* The list of Linux threads cached by linux-kthread.  */
> > +typedef struct private_thread_info
> > +{
> > +  struct private_thread_info *next;
> > +  CORE_ADDR task_struct;
> > +  CORE_ADDR mm;
> > +  CORE_ADDR active_mm;
> > +
> > +  ptid_t old_ptid;
> > +
> > +  /* This is the "dynamic" core info.  */
> > +  int core;
> > +
> > +  int tgid;
> > +  unsigned int prio;
> > +  char *comm;
> > +  int valid;
> > +
> > +  struct thread_info *gdb_thread;
> > +} linux_kthread_info_t;
> > +
> > +#define PTID_OF(ps) ((ps)->gdb_thread->ptid)
> > +
> > +int lkthread_lookup_addr (struct addr_info *field, int check);
> > +int lkthread_lookup_field (struct field_info *field, int check);
> > +
> > +static inline CORE_ADDR
> > +lkthread_get_address (struct addr_info *addr)
> > +{
> > +  if (addr->bmsym.minsym == NULL)
> > +    lkthread_lookup_addr (addr, 0);
> > +
> > +  return BMSYMBOL_VALUE_ADDRESS (addr->bmsym);
> > +}
> > +
> > +static inline unsigned int
> > +lkthread_get_field_offset (struct field_info *field)
> > +{
> > +  if (field->type == NULL)
> > +    lkthread_lookup_field (field, 0);
> > +
> > +  return field->offset;
> > +}
> > +
> > +static inline unsigned int
> > +lkthread_get_field_size (struct field_info *field)
> > +{
> > +  if (field->type == NULL)
> > +    lkthread_lookup_field (field, 0);
> > +
> > +  return field->size;
> > +}
> > +
> > +#define CORE_INVAL (-1)
> > +
> > +#define FIELD_INFO(s_name, field) _FIELD_##s_name##__##field
> > +
> > +#define DECLARE_FIELD(s_name, field)			\
> > +  static struct field_info FIELD_INFO(s_name, field)	\
> > +  = { .struct_name = #s_name, .field_name = #field, 0 }
> > +
> > +#define F_OFFSET(struct, field)					\
> > +  lkthread_get_field_offset (&FIELD_INFO(struct, field))
> > +
> > +#define F_SIZE(struct, field)				\
> > +  lkthread_get_field_size (&FIELD_INFO(struct, field))
> > +
> > +#define HAS_FIELD(struct, field)					\
> > +  (FIELD_INFO(struct, field).type != NULL				\
> > +   || (lkthread_lookup_field(&FIELD_INFO(struct, field), 1),		\
> > +       FIELD_INFO(struct, field).type != NULL))
> > +
> > +#define DECLARE_ADDR(symb)						\
> > +  static struct addr_info symb = { .name = #symb, .bmsym = {NULL, NULL} }
> > +
> > +#define HAS_ADDR(symb)							\
> > +  (symb.bmsym.minsym != NULL						\
> > +   || (lkthread_lookup_addr(&symb, 1), symb.bmsym.minsym != NULL))
> > +
> > +#define HAS_ADDR_PTR(symb)						\
> > +  (symb->bmsym.minsym != NULL						\
> > +   || (lkthread_lookup_addr(symb, 1), symb->bmsym.minsym != NULL))
> > +
> > +#define ADDR(sym) lkthread_get_address (&sym)
> > +
> > +#define ADDR_PTR(sym) lkthread_get_address (sym)
> > +
> > +#define read_unsigned_field(base, struct, field, byteorder)		\
> > +  read_memory_unsigned_integer (base + F_OFFSET (struct, field),	\
> > +				F_SIZE (struct, field), byteorder)
> > +
> > +#define read_signed_field(base, struct, field, byteorder) \
> > +  read_memory_integer (base + F_OFFSET (struct, field),			\
> > +		       F_SIZE (struct, field), byteorder)
> > +
> > +#define read_pointer_field(base, struct, field) \
> > +  read_memory_typed_address (base + F_OFFSET (struct, field),		\
> > +			     builtin_type (target_gdbarch ())->builtin_data_ptr)
> > +
> > +#define read_unsigned_embedded_field(base, struct, field, emb_str,	\
> > +				     emb_field, byteorder)		\
> > +  read_memory_unsigned_integer (base + F_OFFSET (struct, field)		\
> > +				+ F_OFFSET (emb_str, emb_field),	\
> > +				F_SIZE (emb_str, emb_field), byteorder)
> > +
> > +#define read_signed_embedded_field(base, struct, field, emb_str,	\
> > +				   emb_field, byteorder)		\
> > +  read_memory_integer (base + F_OFFSET (struct, field)			\
> > +		       + F_OFFSET (emb_str, emb_field),			\
> > +		       F_SIZE (emb_str, emb_field), byteorder)
> > +
> > +#define read_pointer_embedded_field(base, struct, field, emb_str,	\
> > +				    emb_field)				\
> > +  read_memory_typed_address (base + F_OFFSET (struct, field)		\
> > +			     + F_OFFSET (emb_str, emb_field),		\
> > +			     builtin_type (target_gdbarch ())->builtin_data_ptr)
> > +
> > +#define extract_unsigned_field(base, struct, field, byteorder)		\
> > +  extract_unsigned_integer(base + F_OFFSET (struct, field),		\
> > +			   F_SIZE (struct, field), byteorder)
> > +
> > +#define extract_signed_field(base, struct, field, byteorder)		\
> > +  extract_signed_integer (base + F_OFFSET (struct, field),		\
> > +			  F_SIZE (struct, field), byteorder)
> > +
> > +#define extract_pointer_field(base, struct, field)			\
> > +  extract_typed_address (base + F_OFFSET (struct, field),		\
> > +			 builtin_type(target_gdbarch ())->builtin_data_ptr)
> > +
> > +/* Mimic kernel macros.  */
> > +#define container_of(ptr, struc, field)  ((ptr) - F_OFFSET(struc, field))
> > +
> > +
> > +/* Mapping GDB PTID to Linux PID and Core
> > +
> > +   GDB Remote uses LWP to store the effective cpu core
> 
> Why?

Not sure why GDB remote does this. This comment is really a relic from where
linux-kthread had a different layout to GDB remote and some conversion
was needed. I think this comment can be removed now.

> 
> > +   ptid.pid = Inferior PID
> > +   ptid.lwp = CPU Core
> > +   ptid.tid = 0
> > + 
> > +   We store Linux PID in TID.  */
> 
> Why don't you implement target_ops to_core_of_thread?
> 
Yes could look at implementing to_core_of_thread() callback.

regards,

Peter.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]