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

Philipp Rudo
2017-02-09T09:54:43Z

Hi Yao,

On Tue, 7 Feb 2017 17:39:44 +0000
Yao Qi <qiyaoltc@gmail.com> wrote:

> On 17-02-07 16:04:44, Philipp Rudo wrote:
> > 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?
>
> Yes, lk-low.o needs to be added to ALL_TARGET_OBS, but that is not
> enough. I get the following compile error on x86_64-linux,
>
> lk-lists.o: In function `lk_find':
> /home/yao/SourceCode/gnu/gdb/git/gdb/lk-low.h:125: undefined
> reference to
> `linux_kernel_ops' /home/yao/SourceCode/gnu/gdb/git/gdb/lk-low.h:125:
> undefined reference to `linux_kernel_ops' lk-lists.o: In function
> `lk_list_head_next(unsigned long, char const*, char
> const*)': /home/yao/SourceCode/gnu/gdb/git/gdb/lk-lists.c:35:
> undefined reference to `lk_read_addr(unsigned
> long)' /home/yao/SourceCode/gnu/gdb/git/gdb/lk-lists.c:36: undefined
> reference to `lk_read_addr(unsigned long)'
>
> because lk-low.o isn't linked into the executable.
>
> I think both lk-low.o and lk-lists.o should be added for each
> $ARCH-linux target in configure.tgt.

Ok, now I get it. Thanks for pointing this out I simply forgot to test
without --enable-targets=all...

While fixing this I also noticed that I forgot adding the new
s390-tdep.o in my split up patch in v2. I will fix it in v3.

> > > > (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.
>
> Do you have an example that lookup_symbol doesn't work but parse_exp
> works?

No sorry not anymore it got lost in some cleanup. I did it quite early
in development and can't remember everything but I guess that it was
either a memory corruption (I had an use-after-relocate bug in my htab
handling) or that I just didn't understood GDB's symbolhandling when I
stared working on the project.

Nevertheless I looked at it again and it works. In addition it is much
nicer and shorter. The only thing I don't really like are the
"goto out/error" in lk_init_struct but that is just a minor flaw. The
two functions now look like this

struct lk_private_data *
lk_init_addr (const char *name, const char *alias, int silent)
{
struct lk_private_data *data;
struct bound_minimal_symbol bmsym;
void **new_slot;
void *old_slot;

if ((old_slot = lk_find (alias)) != NULL)
return (struct lk_private_data *) old_slot;

bmsym = lookup_minimal_symbol (name, NULL, NULL);

if (bmsym.minsym == NULL)
{
if (!silent)
error (_("Could not find address %s. Aborting."), alias);

return NULL;
}

data = XCNEW (struct lk_private_data);
data->alias = alias;
data->data.addr = BMSYMBOL_VALUE_ADDRESS (bmsym);

new_slot = lk_find_slot (alias);
*new_slot = data;

return data;
}

struct lk_private_data *
lk_init_struct (const char *name, const char *alias, int silent)
{
struct lk_private_data *data;
const struct block *global;
const struct symbol *sym;
struct type *type;
void **new_slot;
void *old_slot;

if ((old_slot = lk_find (alias)) != NULL)
return (struct lk_private_data *) old_slot;

global = block_global_block(get_selected_block (0));
sym = lookup_symbol (name, global, STRUCT_DOMAIN, NULL).symbol;

if (sym != NULL)
{
type = SYMBOL_TYPE (sym);
goto out;
}

/* Chek for "typedef struct { ... } name;"-like definitions. */
sym = lookup_symbol (name, global, VAR_DOMAIN, NULL).symbol;
if (sym == NULL)
goto error;

type = check_typedef (SYMBOL_TYPE (sym));

if (TYPE_CODE (type) == TYPE_CODE_STRUCT)
goto out;

error:
if (!silent)
error (_("Could not find %s. Aborting."), alias);

return NULL;

out:
data = XCNEW (struct lk_private_data);
data->alias = alias;
data->data.type = type;

new_slot = lk_find_slot (alias);
*new_slot = data;

return data;
}

> > > (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.
> >
>
> How is it related to kernel modules? I don't want to test kernel
> module handling in this test case.

When you are only interested in threads it isn't related at all. My
intention was to indicate that when you want to have test cases for the
full target, including module handling, you will need to somehow cope
with it. Although I'm not sure if this approach is the best for
modules. Besides the relocation problem, they also live in kernel
virtual memory. Thus you'd need a test case which simulates it's own
address space...
In my opinion both functions need to be tested when the code is
integrated to master. Otherwise GDB might print wrong and misleading
information to the user without noticing it. Although I must admit
that writing working test cases won't be easy.

Thanks
Philipp