From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 129865 invoked by alias); 19 Mar 2018 00:11:25 -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 129854 invoked by uid 89); 19 Mar 2018 00:11:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-15.4 required=5.0 tests=BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Mar 2018 00:11:22 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 6C4AF1E4B2; Sun, 18 Mar 2018 20:11:20 -0400 (EDT) Subject: Re: [RFC PATCH v5 4/9] Add basic Linux kernel support To: Philipp Rudo , gdb-patches@sourceware.org Cc: Omair Javaid , Yao Qi , arnez@linux.vnet.ibm.com References: <20180312153115.47321-1-prudo@linux.vnet.ibm.com> <20180312153115.47321-5-prudo@linux.vnet.ibm.com> From: Simon Marchi Message-ID: <2de69183-5e87-1aed-3a6a-444b1bb80f92@simark.ca> Date: Mon, 19 Mar 2018 00:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180312153115.47321-5-prudo@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00348.txt.bz2 On 2018-03-12 11:31 AM, Philipp Rudo wrote: > Implement the basic infrastructure and functionality to allow Linux kernel > debugging with GDB. This contains handling of kernel symbols and data > structures as well as a simple target_ops to hook into GDB. For the code > to work architectures must provide an implementation for the virtual > methods in linux_kernel_ops. > > For simplicity this patch only supports static targets, i.e. core dumps. > Support for live debugging will be provided in a separate patch. Hi Phlipp, I'm going back and forth trying to understand how this interacts with the rest of GDB. Meanwhile, could you explain a little bit how this (well, the whole patchset) is expected to be used, from the user point of view? How do you setup a coredump debugging session, and eventually a live remote one? Does the user see one or multiple inferiors? What threads do they see, only kernel threads, or kernel + userspace? Here some minor/formatting things I noted while reading the code, it's not meant to be a full review (I don't feel like I understand well enough yet), but I thought I would share them anyway since I wrote them. > +/* Helper function for try_declare_type. Returns type on success or NULL on > + failure */ > + > +static struct type * > +lk_find_type (const std::string &name) > +{ > + const struct block *global; > + const struct symbol *sym; > + > + global = block_global_block(get_selected_block (0)); Missing space. > + sym = lookup_symbol (name.c_str (), global, STRUCT_DOMAIN, NULL).symbol; > + if (sym != NULL) > + return SYMBOL_TYPE (sym); > + > + /* Chek for "typedef struct { ... } name;"-like definitions. */ "Check" > + /* True when all fiels have been parsed. */ "all fields" > + bool empty () const > + { return m_end == std::string::npos; } > + > + /* Return the depth, i.e. number of fields, in m_alias. */ > + unsigned int depth () const > + { > + size_t pos = m_alias.find (delim); > + unsigned int ret = 0; > + > + while (pos != std::string::npos) > + { > + ret ++; > + pos = m_alias.find (delim, pos + delim.size ()); > + } > + > + return ret; > + } > + > +private: > + /* Alias originally passed to parser. */ > + std::string m_alias; > + > + /* First index of current field in m_alias. */ > + size_t m_start = 0; > + > + /* Last index of current field in m_alias. */ > + size_t m_end = 0; > + > + /* Type of the last field found. Needed to get s_name of embedded > + fields. */ > + struct type *m_last_type = NULL; > + > + /* Delemiter used to separate fields. */ Delimiter. > +/* Helper functions to read and return basic types at a given ADDRess. */ > + > +/* Read and return the integer value at address ADDR. */ Comments for non-static functions should be /* See lk-low.h. */ and documented in lk-low.h, there are a few instances. > --- /dev/null > +++ b/gdb/lk-low.h > @@ -0,0 +1,335 @@ > +/* 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 > + > +#include "gdbtypes.h" > +#include "target.h" > + > +/* Copied 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 > + > +/* Helper functions to read and return a value at a given ADDRess. */ > +extern int lk_read_int (CORE_ADDR addr); > +extern unsigned int lk_read_uint (CORE_ADDR addr); > +extern LONGEST lk_read_long (CORE_ADDR addr); > +extern ULONGEST lk_read_ulong (CORE_ADDR addr); > +extern CORE_ADDR lk_read_addr (CORE_ADDR addr); > + > +/* Enum to track the config options used to build the kernel. Whenever > + a symbol is declared (in linux_kernel_ops::{arch_}read_symbols) which > + only exists if the kernel was built with a certain config option an entry > + has to be added here. */ > +enum lk_kconfig_values > +{ > + LK_CONFIG_ALWAYS = 1 << 0, > + LK_CONFIG_SMT = 1 << 1, > + LK_CONFIG_MODULES = 1 << 2, > +}; > +DEF_ENUM_FLAGS_TYPE (enum lk_kconfig_values, lk_kconfig); > + > +/* 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 the > + target beneath. Otherwise it is impossible to pass jobs, e.g. fetching > + registers of running tasks, to the target beneath. */ > + > +/* Private data struct to map between our and the target beneath PTID. */ > + > +struct lk_ptid_map > +{ > + struct lk_ptid_map *next; > + unsigned int cpu; > + ptid_t old_ptid; I don't really understand the usage of the name "old_ptid". If it's the ptid of the target beneath, why call it "old" and not "beneath_ptid", for example? It makes it sound like its value changed in time. > + /* Check whether the kernel was build using this config option. */ "was built" > + /* Collection of all declared symbols (addresses, fields etc.). */ > + std::unordered_map m_symbols; Is there a reason to put all symbols in the same map? It creates the risk of misusing a symbol using the wrong type, e.g.: try_declare_address ("foo", "foo") then later m_symbols.field ("foo") Is there a reason not to use three different maps? Thanks, Simon