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

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

On Thu, Apr 20 2017, Omair Javaid wrote:

> Hi Philipp and Andreas,
>
> I have some further comments on this patch specifically about copying
> task_struct->pid into ptid->lwp and using task_struct address as tid.
>
> I see that we are overriding lwp, tid which any target beneath might
> be using differently.
>
> So suggestion about storing task_struct->pid or task_struct address is
> to use private_thread_info in binutils-gdb/gdb/gdbthread.h for this
> information.

The current version of the patch series is mainly focused on dump
targets.  Remote targets require some additional changes.  We've
discussed the use of private_thread_info before, and the last I've seen
is that it is not suitable either, because remote.c uses it already:

  https://sourceware.org/ml/gdb-patches/2017-02/msg00543.html

In my view, the private_thread_info field really is a hack, and we are
now facing its limits.  It provides some space for a single thread layer
to store information into, but not for multiple such layers.  In the
case of the Linux kernel we at least have two different thread layers:
the CPU layer (each "thread" is a CPU), and the kernel task layer on top
of that.

I think we need to allow a target to maintain its *own* thread list.
The CPU "thread list" would be maintained by the target beneath
(remote/dump), and the kernel task list would be maintained by the LK
target.  The ptid namespaces could be completely separate.

> I also have reservation about use of old_ptid naming in struct
> lk_private and struct lk_ptid_map.
>
> old_ptid naming is a little confusing kindly choose a distinguishable
> name for old_ptid varibles in both lk_private and lk_ptid_map.
>
> Further Here's an implementation of bitmap_weight function from linux
> kernel. Kindly see if your implementation can be improved and moved to
> a generic area in gdb.
>
>  10 int __bitmap_weight(const unsigned long *bitmap, int bits)
>  11 {
>  12         int k, w = 0, lim = bits/BITS_PER_LONG;
>  13
>  14         for (k = 0; k < lim; k++)
>  15                 w += hweight_long(bitmap[k]);
>  16
>  17         if (bits % BITS_PER_LONG)
>  18                 w += hweight_long(bitmap[k] & BITMAP_LAST_WORD_MASK(bits));
>  19
>  20         return w;
>  21 }

The __bitmap_weight function is specific to Linux, so I'm not sure we
want to move it to a generic area.  For big-endian targets the function
depends on the width of Linux' "unsigned long" type, because
BITMAP_LAST_WORD_MASK builds a mask for the *least significant* bits
instead of the *lowest-addressed* ones.

It's probably true that the performance of lk_bitmap_hweight could be
improved.  For instance, with some care a function like popcount_hwi()
in GCC's hwint.h could be exploited, even if the target's word width and
byte order may not match the GDB client's.  This would not make the
function simpler, though.

--
Andreas