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

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

Hi Omair,

sorry for the late reply but I was sick the last two weeks...

On Mon, 17 Apr 2017 03:58:35 +0500
Omair Javaid <omair.javaid@linaro.org> wrote:

> Hi Philip,
> 
> I like your handling of linux kernel data structures though I havent
> been able to get your code working on arm.

I'm glad to hear it.

> There are some challenges with regards to live debugging support which
> I am trying to figure out. There is no reliable way to tell between a
> kernel direct mapped address, vmalloc address and module address when
> we also have user address available.

I'm afraid you will find more challenges like these.  Although I tried to be as
general as possible I most likely missed some things you will need for live
debugging ...

I don't think there is a reliable way to tell what kind of address you have
when you only have its unsigned long value.  On s390 (at least in theory) it
should be possible to find out via lowcore ("percpu state").  There the
currently running task is stored.  So you could try something like
lowcore->current_task->(mm|active_mm).

I actually never needed to find out what kind of address I have as for dumps the
user space is usually stripped off.  This makes live a lot easier as both
(direct mapped and vmalloc'ed addresses (including modules)) can be accessed
via the kernels page table (init_mm->pgd).  So you don't have to
care about what kind of address you have.  Furthermore they never get swapped
out.

In theory this method could also be used to access user space memory if you
know the corresponding page table and the memory isn't swapped out.  But here
GDB has the problem, that to_xfer_partial (in gdb/target.h) only gets the
address as unsigned long but has absolutely no information about the address
space it lives in ...

 
> Also there is no way to switch between stratums if we need to do so in
> case we try to support switching between userspace and kernel space.

No, GDB currently doesn't allow switching between straums.  This is one point
on my long ToDo-list...

> As far as this patch is concerned there are no major issues that can
> block me from further progressing towards live debugging support.
> 
> I have compiled this patch with arm support on top and overall this
> looks good. See some minor inline comments.
> 
> Yao: Kindly check if there are any coding convention or styling issues here.
> 
> PS: I have not looked at module support or s390 target specific code in
> detail.

No Problem just do one step after the other and don't waste too much time on
s390.  The architecture has some quirks ;)

> Thanks!
> 
> --
> Omair
> 
> 

[...]

> > +
> > +size_t
> > +lk_bitmap_find_next_bit (ULONGEST *bitmap, size_t size, size_t bit)
> > +{
> > +  size_t ulong_size, bits_per_ulong, elt;
> > +
> > +  ulong_size = lk_builtin_type_size (unsigned_long);
> > +  bits_per_ulong = ulong_size * LK_BITS_PER_BYTE;
> > +  elt = bit / bits_per_ulong;
> > +
> > +  while (bit < size)
> > +    {  
> 
> Will this be portable across endianess?

The generic implementation for the bitmap functions in the kernel rely on
BITMAP_LAST_WORD_MASK.  As Andreas mentioned earlier this macro creates a mask
for the least significant bits.  So this implementation should be portable. At
least it works on s390, which is big-endian.
 
> > +      /* FIXME: Explain why using lsb0 bit order.  */
> > +      if (bitmap[elt] & (1UL << (bit % bits_per_ulong)))
> > +       return bit;
> > +
> > +      bit++;
> > +      if (bit % bits_per_ulong == 0)
> > +       elt++;
> > +    }
> > +
> > +  return size;
> > +}
> > +  
> 
> lk_bitmap_hweight seems un-used.
> I wonder if there is generic implementation available for this
> function somewhere in binutils-gdb sources.
> Can we use something like __builtin_popcount from GCC intrinsic?

See reply to your next mail.
 
> > +/* Returns the Hamming weight, i.e. number of set bits, of bitmap BITMAP
> > +   with size SIZE (in bits).  */
> > +
> > +size_t
> > +lk_bitmap_hweight (ULONGEST *bitmap, size_t size)
> > +{
> > +  size_t ulong_size, bit, bits_per_ulong, elt, retval;
> > +
> > +  ulong_size = lk_builtin_type_size (unsigned_long);
> > +  bits_per_ulong = ulong_size * LK_BITS_PER_BYTE;
> > +  elt = bit = 0;
> > +  retval = 0;
> > +
> > +  while (bit < size)
> > +    {
> > +      if (bitmap[elt] & (1 << bit % bits_per_ulong))
> > +       retval++;
> > +
> > +      bit++;
> > +      if (bit % bits_per_ulong == 0)
> > +       elt++;
> > +    }
> > +
> > +  return retval;
> > +}
> > +

[...]

> This function throws an error while compiling for arm-linux target on
> x86_64 host.
> 
> lk-low.c: In function ‘void init_linux_kernel_ops()’:
> lk-low.c:812:20: error: invalid conversion from ‘char*
> (*)(target_ops*, ptid_t)’ to ‘const char* (*)(target_ops*, ptid_t)’
> [-fpermissive]
>    t->to_pid_to_str = lk_pid_to_str;

Could it be that you applied the patch on a master with Pedros "Enable
-Wwrite-strings" series?

https://sourceware.org/ml/gdb-patches/2017-04/msg00028.html

In this series the hook got "const-ified" (7a1149643).  It isn't considered in
my patches yet (aka. need to rebase to the current master, but that will be a
lot of work with all the C++-ification ...).
 
> > +/* Function for targets to_pid_to_str hook.  Marks running tasks with an
> > +   asterisk "*".  */
> > +
> > +static char *
> > +lk_pid_to_str (struct target_ops *target, ptid_t ptid)
> > +{
> > +  static char buf[64];
> > +  long pid;
> > +  CORE_ADDR task;
> > +
> > +  pid = ptid_get_lwp (ptid);
> > +  task = (CORE_ADDR) ptid_get_tid (ptid);
> > +
> > +  xsnprintf (buf, sizeof (buf), "PID: %5li%s, 0x%s",
> > +            pid, ((lk_task_running (task) != LK_CPU_INVAL) ? "*" : ""),
> > +            phex (task, lk_builtin_type_size (unsigned_long)));
> > +
> > +  return buf;
> > +}

[...]

> Nice to have comments for all structs/fields below, a kernel tree
> reference may be?

I'm not 100% sure what you mean.  Is it a comment like
"/* Defined in <linux>/include/linux/sched.h.  */"?

If so I don't think its necessary as you could quickly grep for the
definition.  In particular I am afraid that after a while, when fields get
renamed and moved from one file to an other, the comments will confuse more
than they help.  That's why I haven't added any comments here (besides in what
linux version a field is valid).

> > +  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 the cpu to old ptid map.  Prefer the arch dependent
> > +   map_running_task_to_cpu hook if provided, else assume that the PID used
> > +   by target beneath is the same as in task_struct PID task_struct.  See
> > +   comment on lk_ptid_map in lk-low.h for details.  */
> > +
> > +static void
> > +lk_init_ptid_map ()
> > +{
> > +  struct thread_info *ti;
> > +  ULONGEST *cpu_online_mask;
> > +  size_t size;
> > +  unsigned int cpu;
> > +  struct cleanup *old_chain;
> > +
> > +  if (LK_PRIVATE->old_ptid != NULL)
> > +    lk_free_ptid_map ();
> > +
> > +  size = LK_BITMAP_SIZE (cpumask);
> > +  cpu_online_mask = lk_read_bitmap (LK_ADDR (cpu_online_mask), size);
> > +  old_chain = make_cleanup (xfree, cpu_online_mask);
> > +
> > +  ALL_THREADS (ti)
> > +    {
> > +      struct lk_ptid_map *ptid_map = XCNEW (struct lk_ptid_map);
> > +      CORE_ADDR rq, curr;
> > +      int pid;
> > +
> > +      /* Give the architecture a chance to overwrite default behaviour.  */
> > +      if (LK_HOOK->map_running_task_to_cpu)
> > +       {
> > +         ptid_map->cpu = LK_HOOK->map_running_task_to_cpu (ti);
> > +       }
> > +      else
> > +       {
> > +         LK_BITMAP_FOR_EACH_SET_BIT (cpu_online_mask, size, cpu)
> > +           {
> > +             rq = LK_ADDR (runqueues) + lk_get_percpu_offset (cpu);
> > +             curr = lk_read_addr (rq + LK_OFFSET (rq, curr));
> > +             pid = lk_read_int (curr + LK_OFFSET (task_struct, pid));
> > +
> > +             if (pid == ptid_get_lwp (ti->ptid))
> > +               {
> > +                 ptid_map->cpu = cpu;
> > +                 break;
> > +               }
> > +           }
> > +         if (cpu == size)
> > +           error (_("Could not map thread with pid %d, lwp %lu to a cpu."),
> > +                  ti->ptid.pid, ti->ptid.lwp);  
> 
> Accessing pid and lwp fields directly is not recommended. May be use
> something like
>          error (_("Could not map thread with pid %d, lwp %ld to a cpu."),
>                ptid_get_pid (ti->ptid), ptid_get_lwp (ti->ptid));

Yes, you are right.  I did the quick and dirty solution here because I already
knew that this ptid_map "solution" would only be temporary.  Besides ptid_t got
classified and it's API changed.  So this needs to be adjusted anyway when I
rebase to the current master...

[...]

> > +/* 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.  */  
> 
> ^Typo in comment.

Thanks, fixed.

> Also if its not too much trouble can you kindly put Linux kernel
> source tree references like
> __per_cpu_offset: Linux/include/asm-generic/percpu.h 4.10. in comments.

Like above.  I'm not sure if it is worth the effort and if outdated comments
confuse more than help.
 
[...]

> > +/* Divides numinator N by demoniator D and rounds up the result.  */  
> 
> ^ Spell check above.

What the hell did I do here?  Thanks for the hint, fixed.
 
Hope I didn't miss anything.

Thanks
Philipp