From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91832 invoked by alias); 3 May 2017 15:20:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 91809 invoked by uid 89); 3 May 2017 15:19:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=exploited, client's X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 May 2017 15:19:57 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v43FIfKE050157 for ; Wed, 3 May 2017 11:19:58 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0a-001b2d01.pphosted.com with ESMTP id 2a7hbhs8h8-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 03 May 2017 11:19:58 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 3 May 2017 16:19:55 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (9.149.109.195) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 3 May 2017 16:19:53 +0100 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v43FJqjl15466806; Wed, 3 May 2017 15:19:52 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8FC4F11C064; Wed, 3 May 2017 16:18:44 +0100 (BST) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5F3CB11C050; Wed, 3 May 2017 16:18:44 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 3 May 2017 16:18:44 +0100 (BST) Date: Wed, 03 May 2017 15:20:00 -0000 From: Philipp Rudo To: Omair Javaid Cc: Andreas Arnez , "gdb-patches@sourceware.org" , Yao Qi , Peter Griffin , Lee Jones , Russell Wayman Subject: Re: [RFC v3 3/8] Add basic Linux kernel support In-Reply-To: References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-4-prudo@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17050315-0016-0000-0000-000004908068 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17050315-0017-0000-0000-000027810C52 Message-Id: <20170503171951.559960c9@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-03_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705030284 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00059.txt.bz2 Hi Omair, and now the third mail. On Wed, 3 May 2017 19:12:36 +0500 Omair Javaid wrote: > On 24 April 2017 at 20:24, Andreas Arnez wrote: > > 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. > > Hi Philip and Andreas, > > Further more on this topic, remote stub assigns a common pid to all > CPU threads and uses LWP as CPU number. > While tid is left zero. I think we ll have to rework the old to new > ptid mapping mechanism a little bit in order to adjust all types of > targets beneath. As mentioned in the mail before, the mapping between old and new ptid is a hack. Feel free to change it. Although I think that the only proper solution is to allow every target to manage its own thread_list. > In your implementation of lk_init_ptid_map() you are testing pid from > task_struct against lwp of set by target target beneath. In case of > remote this will never be equal to pid as it is marked as the cpu > number. For dumps it depends on what was written in the dump. Some architectures write there the pid some the cpu (e.g. s390). Thats why I made it possible for the architecture to overwrite the default behavior. I think the cleanest solution is to classify lk_private and make the hooks virtual class methods (as Yao suggested). What do you think? > Also in your implementation of lk_update_running_tasks lwp is being > updated with pid read from task_struct and tid is the task struct > address. > We are doing this not only for linux thread layer tasks but also for > CPU tasks in lk_update_running_tasks. This causes some sync issues > while on live targets as every time we halt a new thread is reported > by remote. > > I think linux thread layer should only update tasks it has created > itself i-e tasks created by parsing task_struct. We can devise a > mechanism to map CPU tasks to curr task_struct and leave CPU tasks as > they were created by target beneath with same ptids. > > Whats your take on this? I'm against this. When we only update the threads we created ourself there will be multiple problems. For example you will see running tasks twice with "info thread", once its task_struct version and once its remote version. Furthermore there will be no mapping between both versions. Thus the task_struct version will show the outdated state when the task was last "unscheduled". While the remote version shows the tasks actual state but has no information about kernel internals, e.g. the linux pid. This would be extremely confusing! In addition there is a chance that you have multiple threads with the same ptid, e.g. cpu1 colliding with the init process with pid = 1. GDB is not able to handle two threads with the same ptid. This could (most likely) be prevented by using the task_struct address as tid. But there is nothing preventing the target beneath to do the same. Long story short. GDBs current thread implementation is a hack with limitations we are now reaching. The only clean solution is for every target to have its own thread_list. Philipp > >> 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 > > >