Re: [patch v6 1/5] x86* unwinder: libebl/

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 17 Oct 2013 14:22:09 +0200, Mark Wielaard wrote:
> On Fri, 2013-10-11 at 22:46 +0200, Jan Kratochvil wrote:
> > +/* Fetch process data from live TID into THREAD->unwound.  */
> > +bool EBLHOOK(set_initial_registers_tid) (pid_t tid,
> > +					 ebl_tid_registers_t *setfunc,
> > +					 void *arg);
> 
> Maybe s/into THREAD->unwound/and call SETFUNC one or more times/ ?

Yes... THREAD is no longer present there anywhere.


> And add something like: "Should only be called when EBL_FRAME_NREGS > 0,
> otherwise the backend doesn't support getting the initial registers."

Used:
	Method should be present only when EBL_FRAME_NREGS > 0, otherwise the
	backend doesn't support unwinding.  */

Updated libdwfl/ dwfl_thread_getframes() so that it reports better error (than
ENOMEM) for EBL_FRAME_NREGS == 0.


> > +/* Callback to fetch process data from live TID.  */
> > +typedef bool (ebl_tid_registers_t) (const int firstreg,
> > +				    unsigned nregs,
> > +				    const Dwarf_Word *regs,
> > +				    void *arg)
> > +  __nonnull_attribute__ (3);
> > +
> > +extern bool ebl_set_initial_registers_tid (Ebl *ebl,
> > +					   pid_t tid,
> > +					   ebl_tid_registers_t *setfunc,
> > +					   void *arg)
> > +  __nonnull_attribute__ (1, 3);
> 
> Actually, the above comment should be added here in libebl.h.

Yes; added for ebl_tid_registers_t:
	/* Callback type for ebl_set_initial_registers_tid.  */

Also added here:
   EBL architecture has to have EBL_FRAME_NREGS > 0, otherwise the
   backend doesn't support unwinding and this function call may crash.  */



> > +/* Number of registers to allocate
> > +   for STATE of ebl_set_initial_registers_tid.  */
> > +extern size_t ebl_frame_nregs (Ebl *ebl)
> > +  __nonnull_attribute__ (1);
> 
> This comment should mention the relationship with the
> ebl_set_initial_registers_tid callback function. If zero, this backend
> doesn't support fetching the initial registers of a tid. Otherwise it
> returns the largest number of DWARF frame registers provided to the
> callback. (Or something similar, I am not sure I worded that right for
> the none-x86 case.)

I do not find the non-x86 case appropriate there now as it is not yet
implemented (it will be in ebl_frame_dwarf_to_regno / frame_dwarf_to_regno).

Put there:
/* Number of registers to allocate for ebl_set_initial_registers_tid.
   EBL architecture can unwind iff EBL_FRAME_NREGS > 0.  */ 


> > +  /* Number of Dwfl_Frame->regs entries to allocate.  */
> > +  size_t frame_nregs;
> 
> If this could be reworded without referring to Dwfl_Frame, that would be
> ideal.

Put there:
  /* Number of registers to allocate for ebl_set_initial_registers_tid. 
     Ebl architecture can unwind iff FRAME_NREGS > 0.  */


> But all the above is just about the comments, I think the actual code is
> good to go now.

Great.  I will check it in only all together.


Thanks,
Jan


libebl/
2013-10-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Mark Wielaard  <mjw@redhat.com>

	* Makefile.am (gen_SOURCES): Add eblinitreg.c.
	* ebl-hooks.h (set_initial_registers_tid): New entry.
	* eblinitreg.c: New file.
	* libebl.h (ebl_tid_registers_t): New definition.
	(ebl_set_initial_registers_tid, ebl_frame_nregs): New declarations.
	* libeblP.h (struct ebl): New entry frame_nregs.

Signed-off-by: Jan Kratochvil <jan.kratochvil@redhat.com>

diff --git a/libebl/Makefile.am b/libebl/Makefile.am
index 4d62fad..4487c5f 100644
--- a/libebl/Makefile.am
+++ b/libebl/Makefile.am
@@ -1,6 +1,6 @@
 ## Process this file with automake to create Makefile.in
 ##
-## Copyright (C) 2000-2010 Red Hat, Inc.
+## Copyright (C) 2000-2010, 2013 Red Hat, Inc.
 ## This file is part of elfutils.
 ##
 ## This file is free software; you can redistribute it and/or modify
@@ -54,7 +54,7 @@ gen_SOURCES = eblopenbackend.c eblclosebackend.c eblstrtab.c \
 	      eblreginfo.c eblnonerelocp.c eblrelativerelocp.c \
 	      eblsysvhashentrysize.c eblauxvinfo.c eblcheckobjattr.c \
 	      ebl_check_special_section.c ebl_syscall_abi.c eblabicfi.c \
-	      eblstother.c
+	      eblstother.c eblinitreg.c
 
 libebl_a_SOURCES = $(gen_SOURCES)
 
diff --git a/libebl/ebl-hooks.h b/libebl/ebl-hooks.h
index d3cf3e6..cb52fee 100644
--- 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,12 @@ 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);
 
+/* Fetch process data from live TID and call SETFUNC one or more times.
+   Method should be present only when EBL_FRAME_NREGS > 0, otherwise the
+   backend doesn't support unwinding.  */
+bool EBLHOOK(set_initial_registers_tid) (pid_t tid,
+					 ebl_tid_registers_t *setfunc,
+					 void *arg);
+
 /* Destructor for ELF backend handle.  */
 void EBLHOOK(destr) (struct ebl *);
diff --git a/libebl/eblinitreg.c b/libebl/eblinitreg.c
new file mode 100644
index 0000000..8909c50
--- /dev/null
+++ b/libebl/eblinitreg.c
@@ -0,0 +1,51 @@
+/* Fetch live process Dwfl_Frame from PID.
+   Copyright (C) 2013 Red Hat, Inc.
+   This file is part of elfutils.
+
+   This file is free software; you can redistribute it and/or modify
+   it under the terms of either
+
+     * the GNU Lesser General Public License as published by the Free
+       Software Foundation; either version 3 of the License, or (at
+       your option) any later version
+
+   or
+
+     * the GNU General Public License as published by the Free
+       Software Foundation; either version 2 of the License, or (at
+       your option) any later version
+
+   or both in parallel, as here.
+
+   elfutils is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   General Public License for more details.
+
+   You should have received copies of the GNU General Public License and
+   the GNU Lesser General Public License along with this program.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#ifdef HAVE_CONFIG_H
+# include <config.h>
+#endif
+
+#include <libeblP.h>
+#include <assert.h>
+
+bool
+ebl_set_initial_registers_tid (Ebl *ebl, pid_t tid,
+			       ebl_tid_registers_t *setfunc,
+			       void *arg)
+{
+  /* Otherwise caller could not allocate THREAD frame of proper size.
+     If set_initial_registers_tid is unsupported then FRAME_NREGS is zero.  */
+  assert (ebl->set_initial_registers_tid != NULL);
+  return ebl->set_initial_registers_tid (tid, setfunc, arg);
+}
+
+size_t
+ebl_frame_nregs (Ebl *ebl)
+{
+  return ebl == NULL ? 0 : ebl->frame_nregs;
+}
diff --git a/libebl/libebl.h b/libebl/libebl.h
index 990167a..622f9e8 100644
--- a/libebl/libebl.h
+++ b/libebl/libebl.h
@@ -1,5 +1,5 @@
 /* Interface for libebl.
-   Copyright (C) 2000-2010 Red Hat, Inc.
+   Copyright (C) 2000-2010, 2013 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -383,6 +383,26 @@ extern int ebl_auxv_info (Ebl *ebl, GElf_Xword a_type,
 			  const char **name, const char **format)
   __nonnull_attribute__ (1, 3, 4);
 
+/* Callback type for ebl_set_initial_registers_tid.  */
+typedef bool (ebl_tid_registers_t) (const int firstreg,
+				    unsigned nregs,
+				    const Dwarf_Word *regs,
+				    void *arg)
+  __nonnull_attribute__ (3);
+
+/* Callback to fetch process data from live TID.
+   EBL architecture has to have EBL_FRAME_NREGS > 0, otherwise the
+   backend doesn't support unwinding and this function call may crash.  */
+extern bool ebl_set_initial_registers_tid (Ebl *ebl,
+					   pid_t tid,
+					   ebl_tid_registers_t *setfunc,
+					   void *arg)
+  __nonnull_attribute__ (1, 3);
+
+/* Number of registers to allocate for ebl_set_initial_registers_tid.
+   EBL architecture can unwind iff EBL_FRAME_NREGS > 0.  */
+extern size_t ebl_frame_nregs (Ebl *ebl)
+  __nonnull_attribute__ (1);
 
 #ifdef __cplusplus
 }
diff --git a/libebl/libeblP.h b/libebl/libeblP.h
index 5ec26a4..4f4137d 100644
--- a/libebl/libeblP.h
+++ b/libebl/libeblP.h
@@ -1,5 +1,5 @@
 /* Internal definitions for interface for libebl.
-   Copyright (C) 2000-2009 Red Hat, Inc.
+   Copyright (C) 2000-2009, 2013 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -60,6 +60,10 @@ struct ebl
   /* Size of entry in Sysv-style hash table.  */
   int sysvhash_entrysize;
 
+  /* Number of registers to allocate for ebl_set_initial_registers_tid.
+     Ebl architecture can unwind iff FRAME_NREGS > 0.  */
+  size_t frame_nregs;
+
   /* Internal data.  */
   void *dlhandle;
 };