This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC v3 3/8] Add basic Linux kernel support
- From: Andreas Arnez <arnez at linux dot vnet dot ibm dot com>
- To: Omair Javaid <omair dot javaid at linaro dot org>
- Cc: Philipp Rudo <prudo at linux dot vnet dot ibm dot com>, "gdb-patches\@sourceware.org" <gdb-patches at sourceware dot org>, Yao Qi <yao dot qi at linaro dot org>, Peter Griffin <peter dot griffin at linaro dot org>, Lee Jones <lee dot jones at linaro dot com>, Russell Wayman <russell dot wayman at linaro dot org>
- Date: Mon, 24 Apr 2017 17:24:03 +0200
- Subject: Re: [RFC v3 3/8] Add basic Linux kernel support
- Authentication-results: sourceware.org; auth=none
- References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-4-prudo@linux.vnet.ibm.com> <CANW4E-0S465AxJCrqtVo632VZX4K-vdAxkvm3NJwtNz-W9Umsw@mail.gmail.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