[PATCH v2] libdw/libdwfl: Add API for accessing registers
Di Chen
dichen@redhat.com
Fri Jul 29 15:18:45 GMT 2022
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 <dichen@redhat.com> 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 <mark@klomp.org> 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 <dichen@redhat.com>
>> > 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 <dichen@redhat.com>
>> > ---
>> > 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
>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libdwfl-Add-new-function-dwfl_frame_reg.patch
Type: text/x-patch
Size: 9239 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/elfutils-devel/attachments/20220729/4facc83b/attachment-0001.bin>
More information about the Elfutils-devel
mailing list