Re-pushed for fixing run-backtrace-core-sparc.sh failure which resulted from some wrong register number. On Fri, Jul 29, 2022 at 9:27 PM Di Chen wrote: > 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 >> >>