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

Philipp Rudo
2017-02-07T15:04:54Z

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