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: <[email protected]> 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 <[email protected]> > > * 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 <[email protected]> > > * 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.
