This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch v4] unwinder: The unwinder (x86* only)
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Tue, 08 Oct 2013 16:43:02 +0200
- Subject: Re: [patch v4] unwinder: The unwinder (x86* only)
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.