Re: [RFC PATCH 4/6] Linux kernel debug infrastructure and target Linux kernel

[John Baldwin <jhb@FreeBSD.org>]

On 1/28/19 9:03 PM, Omair Javaid wrote:
> From: Philipp Rudo <prudo@linux.ibm.com>
> 
> Implements basic infrastructure and functionality to allow Linux kernel
> thread aware debugging with GDB. This is improved version of
> "Add basic Linux kernel support" from patch series submitted by
> Philipp Rudo.
> 
> https://sourceware.org/ml/gdb-patches/2018-03/msg00243.html
> 
> This patch contains implementation of:
> 
> * linux_kernel_ops class which together with other classes and helper
> functions implements Linux kernel task management. This class and its
> various components also handle kernel symbols and data structures required
> for retrieving/updating Linux kernel data. Some parts of linux_kernel_ops
> functionality is target specific and is implemented by target specific
> implementation in subsequent patches.
> 
> * linux_kernel_target class implements Linux kernel target for GDB provides
> overridden implementation for wait, resume, update_thread_list etc.

A couple of thoughts I had:

1) You might be able to simplify some of the code by using 'parse_and_eval_long'
   to read the value of global variables.  This takes care of endianness, etc.
   without requiring you to manually use msymbols, read_memory_* etc.  For
   example, I read some global read-only variables this way that describe
   the offsets of some fields used to enumerate kernel threads in FreeBSD
   using this:

	/*
	 * Newer kernels export a set of global variables with the offsets
	 * of certain members in struct proc and struct thread.  For older
	 * kernels, try to extract these offsets using debug symbols.  If
	 * that fails, use native values.
	 */
	TRY {
		proc_off_p_pid = parse_and_eval_long("proc_off_p_pid");
		proc_off_p_comm = parse_and_eval_long("proc_off_p_comm");
		proc_off_p_list = parse_and_eval_long("proc_off_p_list");
		proc_off_p_threads = parse_and_eval_long("proc_off_p_threads");
		thread_off_td_tid = parse_and_eval_long("thread_off_td_tid");
		thread_off_td_name = parse_and_eval_long("thread_off_td_name");
		thread_off_td_oncpu = parse_and_eval_long("thread_off_td_oncpu");
		thread_off_td_pcb = parse_and_eval_long("thread_off_td_pcb");
		thread_off_td_plist = parse_and_eval_long("thread_off_td_plist");
		thread_oncpu_size = 4;
	} CATCH(e, RETURN_MASK_ERROR) {

2) The code to compute offsetof is more generally useful and is probably
   worth pulling out.  I recently had to do something similar for TLS
   support for FreeBSD userland binaries to deal with offsets in the
   runtime linker link_map.  Currently I'm just relying on a manual
   offsetof, but it would be better to implement a 'lookup_struct_elt_offset'
   helper I think which you have the guts of for at least C.  You can find
   my mail about that here:

   https://sourceware.org/ml/gdb-patches/2019-01/msg00540.html

3) Finally, adding a new field to gdbarch for lk_ops is probably fine.  I
   chose to use gdbarch_data for this instead for the FreeBSD bits.  To do
   this I setup a key and exposed some helper functions to allow a gdbarch
   to set pointers in a similar structure in
   https://github.com/bsdjhb/gdb/blob/kgdb/gdb/fbsd-kvm.c:

/* Per-architecture data key.  */
static struct gdbarch_data *fbsd_vmcore_data;

struct fbsd_vmcore_ops
{
  /* Supply registers for a pcb to a register cache.  */
  void (*supply_pcb)(struct regcache *, CORE_ADDR);

  /* Return address of pcb for thread running on a CPU. */
  CORE_ADDR (*cpu_pcb_addr)(u_int);
};

static void *
fbsd_vmcore_init (struct obstack *obstack)
{
  struct fbsd_vmcore_ops *ops;

  ops = OBSTACK_ZALLOC (obstack, struct fbsd_vmcore_ops);
  return ops;
}

/* Set the function that supplies registers from a pcb
   for architecture GDBARCH to SUPPLY_PCB.  */

void
fbsd_vmcore_set_supply_pcb (struct gdbarch *gdbarch,
			    void (*supply_pcb) (struct regcache *,
						CORE_ADDR))
{
  struct fbsd_vmcore_ops *ops = (struct fbsd_vmcore_ops *)
    gdbarch_data (gdbarch, fbsd_vmcore_data);
  ops->supply_pcb = supply_pcb;
}

/* Set the function that returns the address of the pcb for a thread
   running on a CPU for
   architecture GDBARCH to CPU_PCB_ADDR.  */

void
fbsd_vmcore_set_cpu_pcb_addr (struct gdbarch *gdbarch,
			      CORE_ADDR (*cpu_pcb_addr) (u_int))
{
  struct fbsd_vmcore_ops *ops = (struct fbsd_vmcore_ops *)
    gdbarch_data (gdbarch, fbsd_vmcore_data);
  ops->cpu_pcb_addr = cpu_pcb_addr;
}

...

void
_initialize_kgdb_target(void)
{

	...
	fbsd_vmcore_data = gdbarch_data_register_pre_init(fbsd_vmcore_init);
	...
}

Then the various architecture-specific kernel architectures invoke those
extra methods while setting up an architecture similar to what you are doing,
e.g. from arm-fbsd-kern.c:

/* Implement the 'init_osabi' method of struct gdb_osabi_handler.  */

static void
arm_fbsd_kernel_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
{
  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

  frame_unwind_prepend_unwinder (gdbarch, &arm_fbsd_trapframe_unwind);

  set_solib_ops (gdbarch, &kld_so_ops);

  tdep->jb_pc = 24;
  tdep->jb_elt_size = 4;

  fbsd_vmcore_set_supply_pcb (gdbarch, arm_fbsd_supply_pcb);
  fbsd_vmcore_set_cpu_pcb_addr (gdbarch, kgdb_trgt_stop_pcb);

  /* Single stepping.  */
  set_gdbarch_software_single_step (gdbarch, arm_software_single_step);
}

-- 
John Baldwin