From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 0CE5B384D19B for ; Thu, 21 Jul 2022 14:32:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0CE5B384D19B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: by gnu.wildebeest.org (Postfix, from userid 1000) id 1FAC3300047C; Thu, 21 Jul 2022 16:32:27 +0200 (CEST) Date: Thu, 21 Jul 2022 16:32:27 +0200 From: Mark Wielaard To: Di Chen Cc: elfutils-devel@sourceware.org Subject: Re: [PATCH v2] libdw/libdwfl: Add API for accessing registers Message-ID: <20220721143227.GC7014@gnu.wildebeest.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 14:32:30 -0000 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