From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 121626 invoked by alias); 10 May 2017 09:36:28 -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 121428 invoked by uid 89); 10 May 2017 09:36:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= 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, 10 May 2017 09:36:24 +0000 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v4A9Ihii142491 for ; Wed, 10 May 2017 05:36:26 -0400 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0a-001b2d01.pphosted.com with ESMTP id 2abcjaqv0p-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 10 May 2017 05:36:25 -0400 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 10 May 2017 10:36:22 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (9.149.109.197) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 10 May 2017 10:36:21 +0100 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v4A9aL3032964630; Wed, 10 May 2017 09:36:21 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 75EEF11C05B; Wed, 10 May 2017 10:35:03 +0100 (BST) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 45D4311C04C; Wed, 10 May 2017 10:35:03 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 10 May 2017 10:35:03 +0100 (BST) Date: Wed, 10 May 2017 09:36:00 -0000 From: Philipp Rudo To: Omair Javaid Cc: Yao Qi , "gdb-patches@sourceware.org" , Yao Qi , Peter Griffin , Andreas Arnez Subject: Re: [RFC v3 3/8] Add basic Linux kernel support In-Reply-To: <20170508132204.7a733dc2@ThinkPad> References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-4-prudo@linux.vnet.ibm.com> <86d1br8q8v.fsf@gmail.com> <20170503173626.15e61941@ThinkPad> <20170508132204.7a733dc2@ThinkPad> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17051009-0020-0000-0000-0000036246F0 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17051009-0021-0000-0000-000041A9208F Message-Id: <20170510113619.71b1e5ae@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-10_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705100067 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00244.txt.bz2 Hi Omair, I forgot one thin on my bucket list. * The way the module support maps module names to paths to .ko files should be improved. Currently I parse /modules.order (usually = /lib/modules/$(uname -r)) to do so and load the path relative . The problem is that for ubuntu the files in /lib/modules/... are stripped off their debuginfo. And thus GDB complains that it cannot load the modules symbols. The full files (including debuginfo) can be found under /usr/lib/debug/lib/modules/$(uname -r)/ but this directory doesn't contain the modules.order file ... There is a simple workaround by copying modules.order to /usr/lib/debug/..., nevertheless it would be nicer if the mapping would be more robust. Philipp On Mon, 8 May 2017 13:22:04 +0200 Philipp Rudo wrote: > Hi Omair, > > On Mon, 8 May 2017 04:54:16 +0500 > Omair Javaid wrote: > > > Hi Phillip, > > > > Thanks for writing back. I hope you are feeling better now. > > Thanks. It will take some more time for me to get 100% fit again but at least > the worst is over ... > > > I am trying to manage our basic live thread implementation within the > > limits you have set out in your patches. > > > > However I am interested in knowing what are your plans for immediate > > future like next couple of weeks. > > > > If you are not planning on making any particular design changes to the > > current version of your patches then probably I will continue working > > using your patches as base. > > My current plan is to finish off the work that has piled up during the two > weeks I was sick. After that I will clean up my kernel stack unwinder for > s390 so I have that finally gone (it already took way too much time). > > From then I don't have a fixed plan. On my bucket list there are some items > without particular order and different impact to the interfaces. They are > > * Rebase to current master. > With all the C++-yfication this will most likely lead to some minor changes. > > * C++-fy the target itself. > As Yao mentioned in his mail [1] it would be better to classify > struct lk_private to better fit the direction GDB is currently going to. > In this process I would also get rid of some cleanups and further adept the > new C++ features. Overall this will change some (but hopefully not > many) interfaces. The biggest change will most likely be from function > hooks (in struct lk_private_hooks) to virtual class methods (in lk_private). > > * Make LK_DECLARE_* macros more flexible. > Currently lk_init_private_data aborts once any declared symbol cannot be > found. This also makes the whole target unusable if e.g. the kernel is > compiled without CONFIG_MODULES as then some symbols needed for module > support cannot be found. My idea is to assign symbols to GDB-features e.g. > module support and only turn off those features if a symbol could not be > found. > > * Design a proper CLI (i.e. functions like dmesg etc.). > This will be needed if we want others to actually use the feature. > Shouldn't have any impact on you. > > * Implement separate thread_lists. > Allow every target to manage its own thread_list. Heavy impact for you and > a lot work for me... > > * Implement different target views. > Allow the user to switch between different target views (e.g. linux_kernel > and core/remote) and thus define the wanted level of abstraction. Even > worse then the separate thread_lists... > > Long story short you don't have to divert away from my patches. Even if I > start working on the separate thread_lists next it will definitely take quite > a lot of time to implement. So no matter what you will most likely have a > working patch before me ;) > > I hope I answered all your questions. > > Philipp > > [1] https://sourceware.org/ml/gdb-patches/2017-05/msg00004.html > > > Otherwise if you plan to make any further changes like going for a > > separate thread list implementation for all layers of targets then i > > can also divert away from your patches for a while untill next update > > is posted. > > > > I am already diverting away from Peter's original implementation > > because of some basic limitations pointed out during previous reviews. > > I dont have reliable solution right now but trying to find one lets > > see if i can manage to upgrade this current hack for live threads as > > well. > > > > -- > > Omair. > > > > On 3 May 2017 at 20:36, Philipp Rudo wrote:r > > > Hi Yao, > > > > > > > > > On Tue, 02 May 2017 12:14:40 +0100 > > > Yao Qi wrote: > > > > > >> Philipp Rudo writes: > > >> > > >> Hi Philipp, > > >> > > >> > +/* Initialize architecture independent private data. Must be called > > >> > + _after_ symbol tables were initialized. */ > > >> > + > > >> > +static void > > >> > +lk_init_private_data () > > >> > +{ > > >> > + if (LK_PRIVATE->data != NULL) > > >> > + htab_empty (LK_PRIVATE->data); > > >> > + > > >> > + 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 linux kernel target. */ > > >> > + > > >> > +static void > > >> > +init_linux_kernel_ops (void) > > >> > +{ > > >> > + struct target_ops *t; > > >> > + > > >> > + if (linux_kernel_ops != NULL) > > >> > + return; > > >> > + > > >> > + t = XCNEW (struct target_ops); > > >> > + t->to_shortname = "linux-kernel"; > > >> > + t->to_longname = "linux kernel support"; > > >> > + t->to_doc = "Adds support to debug the Linux kernel"; > > >> > + > > >> > + /* set t->to_data = struct lk_private in lk_init_private. */ > > >> > + > > >> > + t->to_open = lk_open; > > >> > + t->to_close = lk_close; > > >> > + t->to_detach = lk_detach; > > >> > + t->to_fetch_registers = lk_fetch_registers; > > >> > + t->to_update_thread_list = lk_update_thread_list; > > >> > + t->to_pid_to_str = lk_pid_to_str; > > >> > + t->to_thread_name = lk_thread_name; > > >> > + > > >> > + t->to_stratum = thread_stratum; > > >> > + t->to_magic = OPS_MAGIC; > > >> > + > > >> > + linux_kernel_ops = t; > > >> > + > > >> > + add_target (t); > > >> > +} > > >> > + > > >> > +/* Provide a prototype to silence -Wmissing-prototypes. */ > > >> > +extern initialize_file_ftype _initialize_linux_kernel; > > >> > + > > >> > +void > > >> > +_initialize_linux_kernel (void) > > >> > +{ > > >> > + init_linux_kernel_ops (); > > >> > + > > >> > + observer_attach_new_objfile (lk_observer_new_objfile); > > >> > + observer_attach_inferior_created (lk_observer_inferior_created); > > >> > +} > > >> > diff --git a/gdb/lk-low.h b/gdb/lk-low.h > > >> > new file mode 100644 > > >> > index 0000000..292ef97 > > >> > --- /dev/null > > >> > +++ b/gdb/lk-low.h > > >> > @@ -0,0 +1,310 @@ > > >> > +/* Basic Linux kernel support, architecture independent. > > >> > + > > >> > + Copyright (C) 2016 Free Software Foundation, Inc. > > >> > + > > >> > + This file is part of GDB. > > >> > + > > >> > + This program is free software; you can redistribute it and/or > > >> > modify > > >> > + it under the terms of the GNU General Public License as published > > >> > by > > >> > + the Free Software Foundation; either version 3 of the License, or > > >> > + (at your option) any later version. > > >> > + > > >> > + This program is distributed in the hope that it will be useful, > > >> > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > >> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > >> > + GNU General Public License for more details. > > >> > + > > >> > + You should have received a copy of the GNU General Public License > > >> > + along with this program. If not, see > > >> > . */ + > > >> > +#ifndef __LK_LOW_H__ > > >> > +#define __LK_LOW_H__ > > >> > + > > >> > +#include "target.h" > > >> > + > > >> > +extern struct target_ops *linux_kernel_ops; > > >> > + > > >> > +/* Copy constants defined in Linux kernel. */ > > >> > +#define LK_TASK_COMM_LEN 16 > > >> > +#define LK_BITS_PER_BYTE 8 > > >> > + > > >> > +/* Definitions used in linux kernel target. */ > > >> > +#define LK_CPU_INVAL -1U > > >> > + > > >> > +/* Private data structs for this target. */ > > >> > +/* Forward declarations. */ > > >> > +struct lk_private_hooks; > > >> > +struct lk_ptid_map; > > >> > + > > >> > +/* Short hand access to private data. */ > > >> > +#define LK_PRIVATE ((struct lk_private *) linux_kernel_ops->to_data) > > >> > +#define LK_HOOK (LK_PRIVATE->hooks) > > >> > + > > >> > +struct lk_private > > >> > > >> "private" here is a little confusing. How about rename it to > > >> "linux_kernel"? > > > > > > I called it "private" as it is the targets private data stored in its > > > to_data hook. But I don't mind renaming it. Especially ... > > > > > >> > +{ > > >> > + /* Hashtab for needed addresses, structs and fields. */ > > >> > + htab_t data; > > >> > + > > >> > + /* Linked list to map between cpu number and original ptid from > > >> > target > > >> > + beneath. */ > > >> > + struct lk_ptid_map *old_ptid; > > >> > + > > >> > + /* Hooks for architecture dependent functions. */ > > >> > + struct lk_private_hooks *hooks; > > >> > +}; > > >> > + > > >> > > >> Secondly, can we change it to a class and function pointers in > > >> lk_private_hooks become virtual functions. gdbarch_lk_init_private > > >> returns a pointer to an instance of sub-class of "linux_kernel". > > >> > > >> lk_init_private_data can be put the constructor of base class, to add > > >> entries to "data", and sub-class (in each gdbarch) can add their own > > >> specific stuff. > > > > > > ... when classifying the struct, which already is on my long ToDo-list. > > > This struct is a left over from when I started working on the project > > > shortly before gdb-7.12 was released. I didn't think that the > > > C++-yfication would kick off that fast and started with plain C ... > > > > > > Thanks > > > Philipp > > > > > >> > + > > >> > +/* Functions to initialize private data. Do not use directly, use the > > >> > + macros below instead. */ > > >> > + > > >> > +extern struct lk_private_data *lk_init_addr (const char *name, > > >> > + const char *alias, int > > >> > silent); +extern struct lk_private_data *lk_init_struct (const char > > >> > *name, > > >> > + const char *alias, int > > >> > silent); > > >> > > >> > + > > >> > +/* Definitions for architecture dependent hooks. */ > > >> > +/* Hook to read registers from the target and supply their content > > >> > + to the regcache. */ > > >> > +typedef void (*lk_hook_get_registers) (CORE_ADDR task, > > >> > + struct target_ops *target, > > >> > + struct regcache *regcache, > > >> > + int regnum); > > >> > + > > >> > +/* 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. */ > > >> > +typedef CORE_ADDR (*lk_hook_get_percpu_offset) (unsigned int cpu); > > >> > + > > >> > +/* Hook to map a running task to a logical CPU. Required if the > > >> > target > > >> > + beneath uses a different PID as struct rq. */ > > >> > +typedef unsigned int (*lk_hook_map_running_task_to_cpu) (struct > > >> > thread_info *ti); + > > >> > +struct lk_private_hooks > > >> > +{ > > >> > + /* required */ > > >> > + lk_hook_get_registers get_registers; > > >> > + > > >> > + /* optional, required if __per_cpu_offset array is not used to > > >> > determine > > >> > + offset. */ > > >> > + lk_hook_get_percpu_offset get_percpu_offset; > > >> > + > > >> > + /* optional, required if the target beneath uses a different PID as > > >> > struct > > >> > + rq. */ > > >> > + lk_hook_map_running_task_to_cpu map_running_task_to_cpu; > > >> > +}; > > >> > > > > > >