Re: [patch v4] unwinder: The unwinder (x86* only)

[Mark Wielaard <mjw at redhat dot com>]

Hi Jan,

On Mon, 2013-09-02 at 15:26 +0200, Jan Kratochvil wrote:

> here is the refactored version addressing the paragraph from my last post
> 	[patch v3] unwinder: The unwinder (x86* only)x
> 	https://lists.fedorahosted.org/pipermail/elfutils-devel/2013-June/003109.html
> 	Message-ID: <20130623183057.GA9934@host2.jankratochvil.net>

This is taking way too long for a review. Sorry.
But it is a fairly big patch. So lets concentrate on some smaller parts
to get things moving. Just looking at the ebl/backend change now.

>     backends/
>     2013-06-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
>     
>     	* Makefile.am (AM_CPPFLAGS): Add ../libdwfl.
>     	(i386_SRCS): Add i386_initreg.c.
>     	(x86_64_SRCS): Add x86_64_initreg.c.
>     	* i386_initreg.c: New file.
>     	* i386_init.c (i386_init): Initialize frame_nregs and
>     	set_initial_registers_tid.
>     	* x86_64_initreg.c: New file.
>     	* x86_64_init.c (x86_64_init): Initialize frame_nregs and
>     	set_initial_registers_tid.
> [...]
>     libebl/
>     2013-06-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
>     
>     	* Makefile.am (AM_CPPFLAGS): Add ../libdwfl.
>     	(gen_SOURCES): Add eblinitreg.c.
>     	* ebl-hooks.h (set_initial_registers_tid): New entry.
>     	* eblinitreg.c: New file.
>     	* libebl.h (dwfl_thread_state_registers_t): New definition.
>     	(ebl_set_initial_registers_tid, ebl_frame_nregs): New declarations.
>     	* libeblP.h (Dwfl_Module): New declaration.
>     	(struct ebl): New entry frame_nregs.

I don't really like that ebl/backends now depends on dwfl. If only
because it makes just reviewing the ebl/backends changes harder :)
It feels like the code layering is breached by having the low-level
library code depend on the high-level library definitions.

This is needed because the new ebl_set_initial_registers_tid function is
defined as:

diff --git a/libebl/libebl.h b/libebl/libebl.h
> index cae31c9..b11c4b3 100644
> --- a/libebl/libebl.h
> +++ b/libebl/libebl.h
> [...]
> +/* Fetch process data from live TID into THREAD->unwound.  */
> +struct Dwfl_Thread;
> +typedef bool (dwfl_thread_state_registers_t) (struct Dwfl_Thread *thread,
> +					      const int firstreg,
> +					      unsigned nregs,
> +					      const Dwarf_Word *regs)
> +  __nonnull_attribute__ (1, 4);
> +extern bool ebl_set_initial_registers_tid (struct Dwfl_Thread *thread,
> +					   pid_t tid,
> +					 dwfl_thread_state_registers_t *setfunc)
> +  __nonnull_attribute__ (1);

And Dwfl_Thread is defined in libdwfl.h.

ebl_set_initial_registers does two things. First it determines (from the
Dwfl_Thread) which tid it wants, and fetches the registers for it for
that tid. Then it puts the registers that will be needed for frame
unwinding into a Dwarf_Word[] and calls the setfunc for on it.

I was wondering if you had considered doing this more like is done for
core files to make it a bit more generic?

For core files we have ebl_core_note () that gives you a pointer to the
register "block", the number of registers and an array of
Ebl_Register_Location describing the registers (offset in the block,
DWARF register number, bits, etc).

The advantage would be that things are a little bit more flexible for
the caller and that more code could be shared between things that
process registers in core notes and those that grab them from a process.
And that we don't have a strict reliance on the libdwfl data structures.
The disadvantage of course is that it is a bit more work to extract all
that information.

Does that make sense? If so I can try to rewrite the code a bit to do
that.

Thanks,

Mark


> --- a/libebl/ebl-hooks.h
> +++ b/libebl/ebl-hooks.h
> @@ -1,5 +1,5 @@
>  /* Backend hook signatures internal interface for libebl.
> -   Copyright (C) 2000-2011 Red Hat, Inc.
> +   Copyright (C) 2000-2011, 2013 Red Hat, Inc.
>     This file is part of elfutils.
>  
>     This file is free software; you can redistribute it and/or modify
> @@ -155,5 +155,15 @@ int EBLHOOK(disasm) (const uint8_t **startp, const uint8_t *end,
>     Function returns 0 on success and -1 on error.  */
>  int EBLHOOK(abi_cfi) (Ebl *ebl, Dwarf_CIE *abi_info);
>  
> +/* *SYM must be STT_FUNC.  Then if it describes a function descriptor (PPC64)
> +   convert in-place its data and return a possibly different new name for it.
> +   The name is valid as long as EBL is valid.  */
> +const char *EBLHOOK(get_func_pc) (Ebl *ebl, struct Dwfl_Module *mod,
> +				  GElf_Sym *sym);

BTW. This hook isn't actually implemented in this patch.