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

[Simon Marchi <simark at simark dot ca>]

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