Thanks for the review, Mark. Re-pushed a patch for it. 1. NEWS entry added. 2. __libdwfl_frame_reg_get/dwfl_frame_reg updated with return int. 3. New DWFL_ERROR added: REGISTER_VAL_UNKNOWN. On Thu, Jul 21, 2022 at 10:32 PM Mark Wielaard wrote: > Hi, > > On Tue, Jul 19, 2022 at 10:21:21PM +0800, Di Chen via Elfutils-devel wrote: > > From 9c25b08e46c2031b569a85f91713d009b83f4c26 Mon Sep 17 00:00:00 2001 > > From: Di Chen > > Date: Tue, 19 Jul 2022 14:54:45 +0800 > > Subject: [PATCH] libdw/libdwfl: Add API for accessing registers > > > > Dwfl has most of the infrastructure to keep the full unwind state, > > including the state of unwound registers per frame using > > Dwfl_Thread_Callbacks. But there is no public API to access the state, > > except for the PC (dwfl_frame_pc). > > > > This update renames previous state_get_reg() (in libdwfl/frame_unwind.c) > > to dwfl_frame_reg(), adds a regno check, and makes it a public API. > > This looks mostly fine, some comments below. But it is hard to apply > since your mailer seems to break up lines. If you cannot use git > send-email to send patches it might be easier to use git format-patch > and attach the result. Or to use the sourcehut mirror to create a > fork, push your patches there and let sourcehut sent the patches as > email: https://git.sr.ht/~sourceware/elfutils > > > Signed-off-by: Di Chen > > --- > > libdw/libdw.map | 1 + > > libdwfl/dwfl_frame_regs.c | 18 ++++++++++++++++++ > > libdwfl/frame_unwind.c | 21 +++++---------------- > > libdwfl/libdwfl.h | 4 ++++ > > libdwfl/libdwflP.h | 2 ++ > > 5 files changed, 30 insertions(+), 16 deletions(-) > > > > diff --git a/libdw/libdw.map b/libdw/libdw.map > > index 6da25561..8f393438 100644 > > --- a/libdw/libdw.map > > +++ b/libdw/libdw.map > > @@ -370,4 +370,5 @@ ELFUTILS_0.186 { > > ELFUTILS_0.188 { > > global: > > dwfl_get_debuginfod_client; > > + dwfl_frame_reg; > > } ELFUTILS_0.186; > > Good. Could you also add a NEWS entry for the new public function? > > > diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c > > index 83b1abef..92c4a692 100644 > > --- a/libdwfl/dwfl_frame_regs.c > > +++ b/libdwfl/dwfl_frame_regs.c > > @@ -59,3 +59,21 @@ dwfl_thread_state_register_pc (Dwfl_Thread *thread, > > Dwarf_Word pc) > > state->pc_state = DWFL_FRAME_STATE_PC_SET; > > } > > INTDEF(dwfl_thread_state_register_pc) > > + > > +bool > > +dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val) > > +{ > > + if ((state->regs_set[regno / sizeof (*state->regs_set) / 8] > > + & ((uint64_t) 1U << (regno % (sizeof (*state->regs_set) * 8)))) > == > > 0) > > + { > > + __libdwfl_seterrno (DWFL_E_INVALID_REGNO); > > + return false; > > + } > > + if (! __libdwfl_frame_reg_get (state, regno, val)) > > + { > > + __libdwfl_seterrno (DWFL_E_INVALID_REGISTER); > > + return false; > > + } > > + return true; > > +} > > I see what you want to do here. First check if the register is > actually saved in the frame, if not return an DWFL_E_INVALID_REGNO. If > it is set, then get the value and return it (or if that fails, return > a different error DWFL_E_INVALID_REGISTER). > > But dwfl_frame_reg takes a DWARF register number, but the regs_set > array uses an internal numbering. You first have to translate the > numbering using ebl_dwarf_to_regno. > > Also we might want to give a better error than DWFL_E_INVALID_REGNO > when the register is simply unknown. DWFL_E_REGISTER_VAL_UNKNOWN > maybe? > > The real problem is that __libdwfl_frame_reg_get returns a boolean, > true if things worked fine and the register value is known, false if > the register value is unknown or the DWARF register number was invalid > or some other error occured. > > It might be an idea to change the interface of __libdwfl_frame_reg_get > (and dwfl_frame_reg) to return an int. With zero as success. -1 for an > error (invalid register number) and 1 for a valid DWARF register > value, but the register value for the given frame is > unknown. __libdwfl_frame_reg_get could then also call > __libdwfl_seterrno itself. > > This does need a few more code changes though. Changing all calls to > __libdwfl_frame_reg_get with if (__libdwfl_frame_reg_get (state, > regno, &val) == 0). And remove any redundant __libdwfl_seterrno calls. > > > +INTDEF(dwfl_frame_reg) > > Good. Now you can use INTUSE (dwfl_frame_reg) for internal calls. > > > diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c > > index 9ac33833..38f5b332 100644 > > --- a/libdwfl/frame_unwind.c > > +++ b/libdwfl/frame_unwind.c > > @@ -78,17 +78,6 @@ __libdwfl_frame_reg_set (Dwfl_Frame *state, unsigned > > regno, Dwarf_Addr val) > > return true; > > } > > > > -static bool > > -state_get_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Addr *val) > > -{ > > - if (! __libdwfl_frame_reg_get (state, regno, val)) > > - { > > - __libdwfl_seterrno (DWFL_E_INVALID_REGISTER); > > - return false; > > - } > > - return true; > > -} > > - > > static int > > bra_compar (const void *key_voidp, const void *elem_voidp) > > { > > @@ -211,7 +200,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, > const > > Dwarf_Op *ops, > > } > > break; > > case DW_OP_reg0 ... DW_OP_reg31: > > - if (! state_get_reg (state, op->atom - DW_OP_reg0, &val1) > > + if (! dwfl_frame_reg (state, op->atom - DW_OP_reg0, &val1) > > || ! push (val1)) > > { > > So these calls would become: > > if (INTUSE (dwfl_frame_reg) (state, op->atom - DW_OP_reg0, &val1) != 0 > || ! push (val1)) > > > free (stack.addrs); > > @@ -219,14 +208,14 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, > > const Dwarf_Op *ops, > > } > > break; > > case DW_OP_regx: > > - if (! state_get_reg (state, op->number, &val1) || ! push (val1)) > > + if (! dwfl_frame_reg (state, op->number, &val1) || ! push (val1)) > > { > > free (stack.addrs); > > return false; > > } > > break; > > case DW_OP_breg0 ... DW_OP_breg31: > > - if (! state_get_reg (state, op->atom - DW_OP_breg0, &val1)) > > + if (! dwfl_frame_reg (state, op->atom - DW_OP_breg0, &val1)) > > { > > free (stack.addrs); > > return false; > > @@ -239,7 +228,7 @@ expr_eval (Dwfl_Frame *state, Dwarf_Frame *frame, > const > > Dwarf_Op *ops, > > } > > break; > > case DW_OP_bregx: > > - if (! state_get_reg (state, op->number, &val1)) > > + if (! dwfl_frame_reg (state, op->number, &val1)) > > { > > free (stack.addrs); > > return false; > > @@ -591,7 +580,7 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, > Dwarf_CFI > > *cfi, Dwarf_Addr bias) > > else if (reg_ops == NULL) > > { > > /* REGNO is same-value. */ > > - if (! state_get_reg (state, regno, ®val)) > > + if (! dwfl_frame_reg (state, regno, ®val)) > > continue; > > } > > else > > And all tese too. > > > diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h > > index b323e8fb..aba75afe 100644 > > --- a/libdwfl/libdwfl.h > > +++ b/libdwfl/libdwfl.h > > @@ -798,6 +798,10 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid, > > bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool > *isactivation) > > __nonnull_attribute__ (1, 2); > > > > +/* Return *val (register value) for frame STATE. */ > > +bool dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val) > > + __nonnull_attribute__ (1); > > + > > So this would then become: > > /* Get the value of the DWARF register number in the given frame. > Returns zero on success, -1 on error (invalud DWARF register > number) or 1 if the value of the register in the frame is unknown. */ > int dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val) > __nonnull_attribute__ (1); > > > > /* Return the internal debuginfod-client connection handle for the DWFL > > session. > > When the client connection has not yet been initialized, it will be > > done on the > > first call to this function. If elfutils is compiled without support > > for debuginfod, > > diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h > > index 9f598370..a6d4396a 100644 > > --- a/libdwfl/libdwflP.h > > +++ b/libdwfl/libdwflP.h > > @@ -81,6 +81,7 @@ typedef struct Dwfl_Process Dwfl_Process; > > DWFL_ERROR (LIBEBL_BAD, N_("Internal error due to ebl")) \ > > DWFL_ERROR (CORE_MISSING, N_("Missing data in core file")) \ > > DWFL_ERROR (INVALID_REGISTER, N_("Invalid register")) \ > > + DWFL_ERROR (INVALID_REGNO, N_("Invalid register number")) \ > > So maybe call this UNKNOWN_REGISTER_VALUE? > > > DWFL_ERROR (PROCESS_MEMORY_READ, N_("Error reading process memory")) > > \ > > DWFL_ERROR (PROCESS_NO_ARCH, N_("Couldn't find architecture of any > > ELF")) \ > > DWFL_ERROR (PARSE_PROC, N_("Error parsing /proc filesystem")) \ > > @@ -786,6 +787,7 @@ INTDECL (dwfl_getthread_frames) > > INTDECL (dwfl_getthreads) > > INTDECL (dwfl_thread_getframes) > > INTDECL (dwfl_frame_pc) > > +INTDECL (dwfl_frame_reg) > > INTDECL (dwfl_get_debuginfod_client) > > > > /* Leading arguments standard to callbacks passed a Dwfl_Module. */ > > > Cheers, > > Mark > >