From: Simon Marchi <simark@simark.ca>
To: Philipp Rudo <prudo@linux.vnet.ibm.com>, gdb-patches@sourceware.org
Cc: Omair Javaid <omair.javaid@linaro.org>,
Yao Qi <qiyaoltc@gmail.com>,
arnez@linux.vnet.ibm.com
Subject: Re: [RFC PATCH v5 4/9] Add basic Linux kernel support
Date: Mon, 19 Mar 2018 00:11:00 -0000 [thread overview]
Message-ID: <2de69183-5e87-1aed-3a6a-444b1bb80f92@simark.ca> (raw)
In-Reply-To: <20180312153115.47321-5-prudo@linux.vnet.ibm.com>
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 <http://www.gnu.org/licenses/>. */
> +
> +#ifndef __LK_LOW_H__
> +#define __LK_LOW_H__
> +
> +#include <unordered_map>
> +
> +#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<std::string, union lk_symbol> 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
next prev parent reply other threads:[~2018-03-19 0:11 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-12 15:31 [RFC v5 0/9] Add support for Linux kernel debugging Philipp Rudo
2018-03-12 15:31 ` [RFC PATCH v5 6/9] Add commands for linux-kernel target Philipp Rudo
2018-03-12 15:31 ` [RFC PATCH v5 3/9] Add scoped_restore_regcache_ptid Philipp Rudo
2018-03-17 18:08 ` Simon Marchi
2018-03-12 15:31 ` [RFC PATCH v5 4/9] Add basic Linux kernel support Philipp Rudo
2018-03-13 14:09 ` Kamil Rytarowski
2018-03-14 9:48 ` Philipp Rudo
2018-03-14 23:38 ` Kamil Rytarowski
2018-03-19 0:11 ` Simon Marchi [this message]
2018-03-12 15:31 ` [RFC PATCH v5 7/9] Add privileged registers for s390x Philipp Rudo
2018-03-12 15:31 ` [RFC PATCH v5 9/9] Add S390 support for linux-kernel target Philipp Rudo
2018-03-12 15:31 ` [RFC PATCH v5 1/9] Convert substitute_path_component to C++ Philipp Rudo
2018-03-16 2:15 ` Simon Marchi
2018-03-17 20:11 ` Simon Marchi
2018-03-12 15:31 ` [RFC PATCH v5 5/9] Add kernel module support for linux-kernel target Philipp Rudo
2018-03-12 15:31 ` [RFC PATCH v5 2/9] Add libiberty/concat styled concat_path function Philipp Rudo
2018-03-16 2:47 ` Simon Marchi
2018-03-12 15:31 ` [RFC PATCH v5 8/9] Link frame_info to thread_info Philipp Rudo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2de69183-5e87-1aed-3a6a-444b1bb80f92@simark.ca \
--to=simark@simark.ca \
--cc=arnez@linux.vnet.ibm.com \
--cc=gdb-patches@sourceware.org \
--cc=omair.javaid@linaro.org \
--cc=prudo@linux.vnet.ibm.com \
--cc=qiyaoltc@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).