Re: [RFC PATCH v5 4/9] Add basic Linux kernel support

Simon Marchi
2018-03-19T00:11:25Z

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