Re: [RFC 3/7] Add basic Linux kernel support

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

Hi Yao


On Tue, 7 Feb 2017 10:54:03 +0000
Yao Qi <qiyaoltc@gmail.com> wrote:

> On 17-01-12 12:32:13, Philipp Rudo wrote:
> >     (ALL_TARGET_OBS): Add lk-low.o  
> 
> ALL_TARGET_OBS is used with --enable-targets=all, so if we put
> lk-low.o in it, lk-low.o can't be linked into GDB if we don't enable
> all targets.

you kind of lost me here.  As I understand it lk-low.o needs to be
added to ALL_TARGET_OPS.  The thing I could do is adding entries in
configure.tgt.  Although I'm not sure if adding lk-low to all
arch*-*-linux* (like Peter did) is better than adding a general *-*-lk*
target.  The benefit of the first is that the target is added
automatically to all linux builds.  While the second allows more
flexibility and turning kernel debugging on/off as needed. 

I don't have a strong opinion on which way to go.  Do you prefer one
way?

> >     (COMMON_OBS): Add lk-lists.o
> > ---  
> 
> > +
> > +/* 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)
> > +{
> > +  /* Initialize to NULL to silence gcc.  */
> > +  struct value *val = NULL;
> > +  struct lk_private_data *data;
> > +  void **new_slot;
> > +  void *old_slot;
> > +
> > +  if ((old_slot = lk_find (alias)) != NULL)
> > +    return (struct lk_private_data *) old_slot;
> > +
> > +  TRY
> > +    {
> > +      /* Choose global block for search, in case the variable was
> > redefined
> > +	 in the current context.  */
> > +      const struct block *global = block_global_block
> > (get_selected_block (0));
> > +      const char *tmp = name;
> > +      expression_up expr = parse_exp_1 (&tmp, 0, global, 0);  
> 
> Why don't you look up symbol or msymbol?  like Peter's patch does.

I used lookup_symbol in the beginning.  But at some point it
suddenly made problems and couldn't find a symbol.  I actually never
found out what went wrong but to parse_exp solved the problem for me.
And, as it's an init function only used only once per symbol I thought
that adding this extra overhead is excusable.

> > +
> > +      gdb_assert (*tmp == '\0');
> > +      val = evaluate_expression (expr.get ());
> > +    }
> > +  CATCH (except, RETURN_MASK_ALL)
> > +    {
> > +      if (!silent)
> > +	error (_("Could not find address %s. Abort."), alias);
> > +
> > +      return NULL;
> > +    }
> > +  END_CATCH
> > +
> > +  data = XCNEW (struct lk_private_data);
> > +  data->alias = alias;
> > +  data->data.addr = value_address (val);
> > +
> > +  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)
> > +{
> > +  /* Initialize to NULL to silence GCC.  */
> > +  struct value *val = NULL;
> > +  struct lk_private_data *data;
> > +  void **new_slot;
> > +  void *old_slot;
> > +
> > +  if ((old_slot = lk_find (alias)) != NULL)
> > +    return (struct lk_private_data *) old_slot;
> > +
> > +  /* There are two ways to define structs
> > +	o struct name { ... };
> > +	o typedef struct { ... } name;
> > +    Both are used in the linux kernel.  Thus we have to check for
> > both ways.
> > +    We do this by first searching for "struct name" (the "struct "
> > is added
> > +    by macro LK_STRUCT_NAME in lk-low.h) and if not found seach
> > for "name". +
> > +    Note: The alias will always keep its "struct "-prefix, even
> > when
> > +    given explicitely. Besides some weird error messages this has
> > no effect.
> > +  */
> > +retry:
> > +  TRY
> > +    {
> > +      /* Choose global block for search, in case the struct was
> > redefined
> > +	 in the current context.  */
> > +      const struct block *global =
> > block_global_block(get_selected_block (0));
> > +      const char *tmp = name;
> > +      expression_up expr = parse_exp_1 (&tmp, 0, global, 0);  
> 
> Likewise.
> 
> > +
> > +      gdb_assert (*tmp == '\0');
> > +      /* parsing just for 'name' can cause name clashes.  Thus
> > also check for
> > +	 OP_TYPE.  */
> > +      if (expr->elts[0].opcode != OP_TYPE)
> > +	    error ("We just need to get to the catch block");
> > +
> > +      val = evaluate_type (expr.get ());
> > +    }
> > +  CATCH (except, RETURN_MASK_ALL)
> > +    {
> > +      /* 7 = strlen ("struct ").  */
> > +      if (strncmp (name, "struct ", 7) == 0)
> > +	{
> > +	  name += 7;
> > +	  goto retry;
> > +	}
> > +
> > +      if (!silent)
> > +	error (_("Could not find %s. Abort."), alias);
> > +
> > +      return NULL;
> > +    }
> > +  END_CATCH
> > +
> > +  data = XCNEW (struct lk_private_data);
> > +  data->alias = alias;
> > +  data->data.type = value_type (val);
> > +
> > +  new_slot = lk_find_slot (alias);
> > +  *new_slot = data;
> > +
> > +  return data;
> > +}
> > +  
> 
> I am playing your first three patches on x86_64 with some hacks.
> I write a small program with the same linux kernel "signature", and
> want GDB treat it as a linux kernel.
> 
> (gdb) b main
> Breakpoint 1 at 0x4004fa:
> file /home/yao/SourceCode/gnu/gdb/git/gdb/testsuite/gdb.base/linux-kernel.c,
> line 59. (gdb) run Starting
> program: /scratch/yao/gdb/build-git/x86_64/gdb/testsuite/outputs/gdb.base/linux-kernel/linux-kernel
> Could not map thread with pid 28278, lwp 28278 to a cpu.
> 
> I hope we can have a small test case in gdb testsuite to test linux
> kernel debugging.  Is it possible?  We can generate a normal coredump
> to linux-kernel in test, and we can also set up task_struct and expect
> GDB sees some "threads".  What do you think?

Andreas and I had the same idea :-)
I don't know If it really works.  But we think its the most promising
approach for the testsuite.  The biggest problem I see is the
relocation done for the kernel modules.  Here you need to somehow mimic
a dynamic linking done in the kernel.  Which I suppose not to be that
simple, but still doable.

Thanks
Philipp