From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99250 invoked by alias); 16 Jun 2017 10:10:34 -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 99232 invoked by uid 89); 16 Jun 2017 10:10:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_STOCKGEN,KHOP_DYNAMIC,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Jun 2017 10:10:30 +0000 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v5GA8b42025125 for ; Fri, 16 Jun 2017 06:10:33 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0b-001b2d01.pphosted.com with ESMTP id 2b47wtptmm-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 16 Jun 2017 06:10:33 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 16 Jun 2017 11:10:31 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 16 Jun 2017 11:10:29 +0100 Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v5GAASkv41484500; Fri, 16 Jun 2017 10:10:28 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id EE43652041; Fri, 16 Jun 2017 10:07:11 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id BA4035203F; Fri, 16 Jun 2017 10:07:11 +0100 (BST) Date: Fri, 16 Jun 2017 10:10:00 -0000 From: Philipp Rudo To: Yao Qi Cc: gdb-patches@sourceware.org, omair.javaid@linaro.org, yao.qi@linaro.org, peter.griffin@linaro.org, arnez@linux.vnet.ibm.com Subject: Re: [RFC v4 3/9] Add basic Linux kernel support In-Reply-To: <86vanxtguc.fsf@gmail.com> References: <20170612170836.25174-1-prudo@linux.vnet.ibm.com> <20170612170836.25174-4-prudo@linux.vnet.ibm.com> <86vanxtguc.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17061610-0016-0000-0000-000004BEE406 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17061610-0017-0000-0000-000027F055A4 Message-Id: <20170616121026.024f664b@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-06-16_05:,, 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-1706160164 X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg00456.txt.bz2 Hi Yao On Thu, 15 Jun 2017 16:23:39 +0100 Yao Qi wrote: > Philipp Rudo writes: > > > /* Initialize a private data entry for an address, where NAME is the name > > + of the symbol, i.e. variable name in Linux, ALIAS the name used to > > + retrieve the entry from hashtab, and SILENT a flag to determine if > > + errors should be ignored. > > + > > + Returns a pointer to the new entry. In case of an error, either returns > > + NULL (SILENT = TRUE) or throws an error (SILENT = FALSE). If SILENT = > > TRUE > > + the caller is responsible to check for errors. > > + > > + Do not use directly, use LK_DECLARE_* macros defined in lk-low.h > > instead. */ + > > +struct lk_private_data * > > +lk_init_addr (const char *name, const char *alias, int silent) > > s/bool/int/ you are right with that. The reason why I haven't changed it yet was that I wanted to c++-ify the whole file in one go. My problem is that I'm currently also working on an other project, which needs to be finished soon. That's why I can't spend all my time on GDB at the moment. That's why I decided it is better to give Omair a working v4, even when it does not contain all changes. For the same reason I also deferred changes like classifying lk_private, htab -> std::unordered_map, cleanup -> scoped_resore etc. > > +{ > > + struct lk_private_data *data; > > + struct bound_minimal_symbol bmsym; > > + void **new_slot; > > + void *old_slot; > > + > > + if ((old_slot = lk_find (alias)) != NULL) > > + return (struct lk_private_data *) old_slot; > > + > > + bmsym = lookup_minimal_symbol (name, NULL, NULL); > > + > > + if (bmsym.minsym == NULL) > > + { > > + if (!silent) > > + error (_("Could not find address %s. Aborting."), alias); > > + return NULL; > > + } > > + > > + data = XCNEW (struct lk_private_data); > > + data->alias = alias; > > + data->data.addr = BMSYMBOL_VALUE_ADDRESS (bmsym); > > + > > + new_slot = lk_find_slot (alias); > > + *new_slot = data; > > + > > + return data; > > +} > > + > > +/* Same as lk_init_addr but for structs. */ > > + > > +struct lk_private_data * > > +lk_init_struct (const char *name, const char *alias, int silent) > > Likewise, bool silent. > > > +{ > > + struct lk_private_data *data; > > + const struct block *global; > > + const struct symbol *sym; > > + struct type *type; > > + void **new_slot; > > + void *old_slot; > > + > > + if ((old_slot = lk_find (alias)) != NULL) > > + return (struct lk_private_data *) old_slot; > > + > > + global = block_global_block(get_selected_block (0)); > > + sym = lookup_symbol (name, global, STRUCT_DOMAIN, NULL).symbol; > > + > > + if (sym != NULL) > > + { > > + type = SYMBOL_TYPE (sym); > > + goto out; > > + } > > + > > + /* Chek for "typedef struct { ... } name;"-like definitions. */ > > + sym = lookup_symbol (name, global, VAR_DOMAIN, NULL).symbol; > > + if (sym == NULL) > > + goto error; > > + > > + type = check_typedef (SYMBOL_TYPE (sym)); > > + > > + if (TYPE_CODE (type) == TYPE_CODE_STRUCT) > > + goto out; > > These two "goto" can be removed. True, but that would lead to code duplication. That's why I chose the way using the two goto. Without goto the function reads like this struct lk_private_data * lk_init_struct (const char *name, const char *alias, int silent) { struct lk_private_data *data; const struct block *global; const struct symbol *sym; struct type *type; void **new_slot; void *old_slot; if ((old_slot = lk_find (alias)) != NULL) return (struct lk_private_data *) old_slot; global = block_global_block(get_selected_block (0)); sym = lookup_symbol (name, global, STRUCT_DOMAIN, NULL).symbol; if (sym != NULL) { type = SYMBOL_TYPE (sym); } else { /* Chek for "typedef struct { ... } name;"-like definitions. */ sym = lookup_symbol (name, global, VAR_DOMAIN, NULL).symbol; if (sym == NULL) { if (!silent) error (_("Could not find %s. Aborting."), alias); return NULL; } type = check_typedef (SYMBOL_TYPE (sym)); if (TYPE_CODE (type) != TYPE_CODE_STRUCT) { if (!silent) error (_("Could not find %s. Aborting."), alias); return NULL; } } data = XCNEW (struct lk_private_data); data->alias = alias; data->data.type = type; new_slot = lk_find_slot (alias); *new_slot = data; return data; } > > + > > +error: > > + if (!silent) > > + error (_("Could not find %s. Aborting."), alias); > > + > > + return NULL; > > + > > +out: > > + data = XCNEW (struct lk_private_data); > > + data->alias = alias; > > + data->data.type = type; > > + > > + new_slot = lk_find_slot (alias); > > + *new_slot = data; > > + > > + return data; > > +} > > + > > + > > +/* Reads a bitmap at a given ADDRess of size SIZE (in bits). Allocates and > > + returns an array of ulongs. The caller is responsible to free the array > > + after it is no longer needed. */ > > + > > +ULONGEST * > > +lk_read_bitmap (CORE_ADDR addr, size_t size) > > +{ > > + ULONGEST *bitmap; > > + size_t ulong_size, len; > > + > > + ulong_size = lk_builtin_type_size (unsigned_long); > > + len = LK_DIV_ROUND_UP (size, ulong_size * LK_BITS_PER_BYTE); > > + bitmap = XNEWVEC (ULONGEST, len); > > + > > + for (size_t i = 0; i < len; i++) > > + bitmap[i] = lk_read_ulong (addr + i * ulong_size); > > + > > + return bitmap; > > +} > > + > > +/* Return the next set bit in bitmap BITMAP of size SIZE (in bits) > > + starting from bit (index) BIT. Return SIZE when the end of the bitmap > > + was reached. To iterate over all set bits use macro > > + LK_BITMAP_FOR_EACH_SET_BIT defined in lk-low.h. */ > > + > > +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) > > + { > > + /* 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; > > +} > > + > > +/* 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; > > +} > > + > > Could you add some unit tests to these operations to bitmap? I am > thinking that we need a class BitMap or BitVector later. You are right on both (unittests and class BitMap). I will see into it when I c++-ify lk-low. > > +/* Provide the per_cpu_offset of cpu CPU. See comment in lk-low.h for > > + details. */ > > + > > +CORE_ADDR > > +lk_get_percpu_offset (unsigned int cpu) > > +{ > > + size_t ulong_size = lk_builtin_type_size (unsigned_long); > > + CORE_ADDR percpu_elt; > > + > > + /* Give the architecture a chance to overwrite default behaviour. */ > > + if (LK_HOOK->get_percpu_offset) > > + return LK_HOOK->get_percpu_offset (cpu); > > + > > + percpu_elt = LK_ADDR (__per_cpu_offset) + (ulong_size * cpu); > > + return lk_read_addr (percpu_elt); > > +} > > + > > + > > > > + > > +/* Function for targets to_update_thread_list hook. */ > > + > > +static void > > +lk_update_thread_list (struct target_ops *target) > > +{ > > + prune_threads (); > > + lk_update_running_tasks (); > > + lk_update_sleeping_tasks (); > > +} > > + > > +/* Function for targets to_fetch_registers hook. */ > > + > > +static void > > +lk_fetch_registers (struct target_ops *target, > > + struct regcache *regcache, int regnum) > > +{ > > + CORE_ADDR task; > > + unsigned int cpu; > > + > > + task = (CORE_ADDR) regcache_get_ptid (regcache).tid (); > > + cpu = lk_task_running (task); > > + > > + /* Let the target beneath fetch registers of running tasks. */ > > + if (cpu != LK_CPU_INVAL) > > + { > > + struct cleanup *old_inferior_ptid; > > + > > + old_inferior_ptid = save_inferior_ptid (); > > + inferior_ptid = lk_cpu_to_old_ptid (cpu); > > Use make_scoped_restore (&inferior_ptid, lk_cpu_to_old_ptid (cpu));? > > > + linux_kernel_ops->beneath->to_fetch_registers (target, regcache, > > regnum); > > + do_cleanups (old_inferior_ptid); > > + } > > + else > > + { > > + struct gdbarch *gdbarch; > > + unsigned int i; > > + > > + LK_HOOK->get_registers (task, target, regcache, regnum); > > + > > + /* Mark all registers not found as unavailable. */ > > + gdbarch = get_regcache_arch (regcache); > > + for (i = 0; i < gdbarch_num_regs (gdbarch); i++) > > + { > > + if (regcache_register_status (regcache, i) == REG_UNKNOWN) > > + regcache_raw_supply (regcache, i, NULL); > > + } > > + } > > +} > > + > > > + > > +/* Functions to initialize and free target_ops and its private data. As > > well > > This line is too long. Oops. Thanks for the hint. > > + as functions for targets to_open/close/detach hooks. */ > > + > > +/* Check if OBFFILE is a Linux kernel. */ > > + > > > > + > > +/* 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 == ti->ptid.lwp ()) > > + { > > + 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 ()); > > + } > > + ptid_map->old_ptid = ti->ptid; > > + ptid_map->next = LK_PRIVATE->old_ptid; > > + LK_PRIVATE->old_ptid = ptid_map; > > + } > > + > > + do_cleanups (old_chain); > > +} > > + > > > diff --git a/gdb/lk-low.h b/gdb/lk-low.h > > new file mode 100644 > > index 0000000000..be8c5556df > > --- /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 > > +{ > > + /* 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; > > +}; > > + > > +/* We use the following convention for PTIDs: > > + > > + ptid->pid = inferiors PID > > + ptid->lwp = PID from task_stuct > > + ptid->tid = address of task_struct > > + > > + The task_structs address as TID has two reasons. First, we need it > > quite > > + often and there is no other reasonable way to pass it down. Second, it > > + helps us to distinguish swapper tasks as they all have PID = 0. > > + > > + Furthermore we cannot rely on the target beneath to use the same PID as > > the > > + task_struct. Thus we need a mapping between our PTID and the PTID of > > > > + target beneath. Otherwise it is impossible to pass jobs, e.g. fetching > > + registers of running tasks, to the target beneath. */ > > Sorry, I don't understand this design. Can you elaborate? The target beneath reports us a "pid" it thinks is right. For example in a core file the register sections are usually named .reg/XXX where XXX names the thread these registers belong to. In user space this is typically the pid of the thread but for kernel dumps it usually is a cpu-id (this needn't be the logical cpu-id used in the kernel). Because of that the kernel ptid, we generate from task_struct, usually has a different lwp than the same thread reported from the target beneath. Nevertheless we need the target beneath to give us information for running tasks. That's why we need to introduce a mapping between the ptid of the target beneath and the kernel ptid. In short, this represents two different thread views (hardware vs. software threads). Did that answer your question? Or did you mean something different? > > + > > +/* Private data struct to map between our and the target beneath PTID. */ > > + > > +struct lk_ptid_map > > +{ > > + struct lk_ptid_map *next; > > Can you use C++ stl list instead? As I already wrote Omair, my ptid_map was only meant to "somehow work" but never to be permanent. Managing the ptid map will be the main task for live debugging. That's why I think it is best when Omair changes this bit to whatever he needs. Philipp > > + unsigned int cpu; > > + ptid_t old_ptid; > > +}; > > + > > +/* Private data struct to be stored in hashtab. */ > > + > > +struct lk_private_data > > +{ > > + const char *alias; > > + > > + union > > + { > > + CORE_ADDR addr; > > + struct type *type; > > + struct field *field; > > + } data; > > +}; > > + > > +/* Wrapper for htab_hash_string to work with our private data. */ > > + > > +static inline hashval_t > > +lk_hash_private_data (const struct lk_private_data *entry) > > +{ > > + return htab_hash_string (entry->alias); > > +} > > + > > +/* Function for htab_eq to work with our private data. */ > > + > > +static inline int > > +lk_private_data_eq (const struct lk_private_data *entry, > > + const struct lk_private_data *element) > > +{ > > + return streq (entry->alias, element->alias); > > +} > > + > > +/* Wrapper for htab_find_slot to work with our private data. Do not use > > + directly, use the macros below instead. */ > > + > > +static inline void ** > > +lk_find_slot (const char *alias) > > +{ > > + const struct lk_private_data dummy = { alias }; > > + return htab_find_slot (LK_PRIVATE->data, &dummy, INSERT); > > +} > > + > > +/* Wrapper for htab_find to work with our private data. Do not use > > + directly, use the macros below instead. */ > > + > > +static inline struct lk_private_data * > > +lk_find (const char *alias) > > +{ > > + const struct lk_private_data dummy = { alias }; > > + return (struct lk_private_data *) htab_find (LK_PRIVATE->data, &dummy); > > +} > > + > > > + > > +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; > > +}; > > The lk_private_hooks is still not a class. I raised this in v3 review, > https://sourceware.org/ml/gdb-patches/2017-05/msg00004.html >