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

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

Hi John,

your points all originate from my original patch set. So let me try to
give you some background (although I haven't worked on the project for
quite some time and my memory is a little rusty...)

@Omair: I'm still looking at the patches. I'll send my findings in a
separate mail.

> 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) {


Do you mean the lk_read_{int,ulong,...} or the lk_find_address function?
Your example looks more like lk_find_address. There 
parse_and_eval_address could do the trick. While implementing however
I played around quite a lot and I remember using parse_and_eval_* for
some time. But, if I recall correct, there where some problems with
Assembler labels (like _text) and I switched to the current 
implementation.

For lk_read_* I don't think parse_and_eval_* is a proper replacement.
Their purpose is to read arbitrary (dynamically allocated) memory
locations. So to replace them with parse_and_eval you'd first would
have to build some C-like expression which then gets evaluated again.
This sounds like a huge overhead to me.

> 
> 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

Hmm, right. It shouldn't take much to convert lk_find_filed to some
generic offsetof function.

> 
> 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:

For me both ways look pretty much the same. The nice thing about adding
a field to gdbarch is that you don't have to implement the helpers
yourself but let gdbarch.sh do the work for you :-)

Thanks
Philipp

> 
> /* 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);
> }
>