public inbox for elfutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2] libdw/libdwfl: Add API for accessing registers
@ 2022-07-19 14:21 Di Chen
  2022-07-21 14:32 ` Mark Wielaard
  0 siblings, 1 reply; 5+ messages in thread
From: Di Chen @ 2022-07-19 14:21 UTC (permalink / raw)
  To: elfutils-devel

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.

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;
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;
+}
+INTDEF(dwfl_frame_reg)
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))
     {
       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, &regval))
+      if (! dwfl_frame_reg (state, regno, &regval))
  continue;
     }
   else
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);
+
 /* 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"))      \
   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.  */
-- 
2.35.3

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] libdw/libdwfl: Add API for accessing registers
  2022-07-19 14:21 [PATCH v2] libdw/libdwfl: Add API for accessing registers Di Chen
@ 2022-07-21 14:32 ` Mark Wielaard
  2022-07-29 13:27   ` Di Chen
  0 siblings, 1 reply; 5+ messages in thread
From: Mark Wielaard @ 2022-07-21 14:32 UTC (permalink / raw)
  To: Di Chen; +Cc: elfutils-devel

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, &regval))
> +      if (! dwfl_frame_reg (state, regno, &regval))
>   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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] libdw/libdwfl: Add API for accessing registers
  2022-07-21 14:32 ` Mark Wielaard
@ 2022-07-29 13:27   ` Di Chen
  2022-07-29 15:18     ` Di Chen
  0 siblings, 1 reply; 5+ messages in thread
From: Di Chen @ 2022-07-29 13:27 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel

[-- Attachment #1: Type: text/plain, Size: 9301 bytes --]

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, &regval))
> > +      if (! dwfl_frame_reg (state, regno, &regval))
> >   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
>
>

[-- Attachment #2: 0001-libdwfl-Add-new-function-dwfl_frame_reg.patch --]
[-- Type: text/x-patch, Size: 9232 bytes --]

From e4d869081ec34947020e8aa5fc6b6060313476dc Mon Sep 17 00:00:00 2001
From: Di Chen <dichen@redhat.com>
Date: Thu, 28 Jul 2022 16:31:05 +0800
Subject: [PATCH] libdwfl: Add new function dwfl_frame_reg

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 commit adds a new function dwfl_frame_reg to get the value of the
DWARF register number in the given frame.

https://sourceware.org/bugzilla/show_bug.cgi?id=28579

Signed-off-by: Di Chen <dichen@redhat.com>
---
 NEWS                        |  2 ++
 libdw/libdw.map             |  1 +
 libdwfl/dwfl_frame_regs.c   | 12 +++++++++++
 libdwfl/frame_unwind.c      | 40 ++++++++++++++-----------------------
 libdwfl/libdwfl.h           |  6 ++++++
 libdwfl/libdwflP.h          | 10 +++++++---
 libdwfl/linux-core-attach.c |  2 +-
 7 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/NEWS b/NEWS
index 392f2edc..eb1a2c6b 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ debuginfod: Add --disable-source-scan option.
 
 libdwfl: Add new function dwfl_get_debuginfod_client.
 
+libdwfl: Add new function dwfl_frame_reg.
+
 Version 0.187
 
 debuginfod: Support -C option for connection thread pooling.
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;
diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
index 83b1abef..a4bd3884 100644
--- a/libdwfl/dwfl_frame_regs.c
+++ b/libdwfl/dwfl_frame_regs.c
@@ -59,3 +59,15 @@ 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)
+
+int
+dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val)
+{
+  int res = __libdwfl_frame_reg_get (state, regno, val);
+  if (res == -1)
+      __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
+  else if (res == 1)
+      __libdwfl_seterrno (DWFL_E_REGISTER_VAL_UNKNOWN);
+  return res;
+}
+INTDEF(dwfl_frame_reg)
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 9ac33833..ca7de6d5 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -43,21 +43,21 @@
    error.  */
 #define DWARF_EXPR_STEPS_MAX 0x1000
 
-bool
+int
 internal_function
 __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno, Dwarf_Addr *val)
 {
   Ebl *ebl = state->thread->process->ebl;
   if (! ebl_dwarf_to_regno (ebl, &regno))
-    return false;
+    return -1;
   if (regno >= ebl_frame_nregs (ebl))
-    return false;
+    return -1;
   if ((state->regs_set[regno / sizeof (*state->regs_set) / 8]
        & ((uint64_t) 1U << (regno % (sizeof (*state->regs_set) * 8)))) == 0)
-    return false;
+    return 1;
   if (val)
     *val = state->regs[regno];
-  return true;
+  return 0;
 }
 
 bool
@@ -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 (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 (INTUSE (dwfl_frame_reg) (state, op->atom - DW_OP_reg0, &val1) != 0)
 	    {
 	      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 (INTUSE (dwfl_frame_reg) (state, op->atom - DW_OP_reg0, &val1) != 0)
 	    {
 	      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 (INTUSE (dwfl_frame_reg) (state, op->number, &val1) != 0)
 	    {
 	      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, &regval))
+	      if (INTUSE (dwfl_frame_reg) (state, regno, &regval) != 0)
 		continue;
 	    }
 	  else
@@ -638,9 +627,10 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
     }
   if (unwound->pc_state == DWFL_FRAME_STATE_ERROR)
     {
-      if (__libdwfl_frame_reg_get (unwound,
-				   frame->fde->cie->return_address_register,
-				   &unwound->pc))
+      int res = INTUSE (dwfl_frame_reg) (unwound,
+          frame->fde->cie->return_address_register,
+          &unwound->pc);
+      if (res == 0)
 	{
 	  /* PPC32 __libc_start_main properly CFI-unwinds PC as zero.
 	     Currently none of the archs supported for unwinding have
@@ -698,7 +688,7 @@ getfunc (int firstreg, unsigned nregs, Dwarf_Word *regs, void *arg)
   Dwfl_Frame *state = arg;
   assert (firstreg >= 0);
   while (nregs--)
-    if (! __libdwfl_frame_reg_get (state, firstreg++, regs++))
+    if (INTUSE (dwfl_frame_reg) (state, firstreg++, regs++) != 0)
       return false;
   return true;
 }
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index b323e8fb..1230972c 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -798,6 +798,12 @@ 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);
 
+/* 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..964daad5 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 (REGISTER_VAL_UNKNOWN, N_("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"))		      \
@@ -277,13 +278,15 @@ struct Dwfl_Frame
 };
 
 /* Fetch value from Dwfl_Frame->regs indexed by DWARF REGNO.
-   No error code is set if the function returns FALSE.  */
-bool __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
+   No error code is set if the function returns 0,
+   -1 on error (invalud DWARF register number),
+   1 if the value of the register in the frame is unknown.  */
+int __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
 			      Dwarf_Addr *val)
   internal_function;
 
 /* Store value to Dwfl_Frame->regs indexed by DWARF REGNO.
-   No error code is set if the function returns FALSE.  */
+   No error code is set if the function returns TRUE.  */
 bool __libdwfl_frame_reg_set (Dwfl_Frame *state, unsigned regno,
 			      Dwarf_Addr val)
   internal_function;
@@ -786,6 +789,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.  */
diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index f68062f0..ee7afa40 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -257,7 +257,7 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
 	     FIXME: It depends now on their order in core notes.
 	     FIXME: It uses private function.  */
 	  if (regno < nregs
-	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL))
+	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL) == 0)
 	    continue;
 	  Dwarf_Word val;
 	  switch (regloc->bits)
-- 
2.37.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] libdw/libdwfl: Add API for accessing registers
  2022-07-29 13:27   ` Di Chen
@ 2022-07-29 15:18     ` Di Chen
  2022-07-30 23:51       ` Mark Wielaard
  0 siblings, 1 reply; 5+ messages in thread
From: Di Chen @ 2022-07-29 15:18 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: elfutils-devel

[-- Attachment #1: Type: text/plain, Size: 9749 bytes --]

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, &regval))
>> > +      if (! dwfl_frame_reg (state, regno, &regval))
>> >   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
>>
>>

[-- Attachment #2: 0001-libdwfl-Add-new-function-dwfl_frame_reg.patch --]
[-- Type: text/x-patch, Size: 9239 bytes --]

From 5266d753623782fb1397cdd258b5ea5373bd91ab Mon Sep 17 00:00:00 2001
From: Di Chen <dichen@redhat.com>
Date: Thu, 28 Jul 2022 16:31:05 +0800
Subject: [PATCH] libdwfl: Add new function dwfl_frame_reg

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 commit adds a new function dwfl_frame_reg to get the value of the
DWARF register number in the given frame.

https://sourceware.org/bugzilla/show_bug.cgi?id=28579

Signed-off-by: Di Chen <dichen@redhat.com>
---
 NEWS                        |  2 ++
 libdw/libdw.map             |  1 +
 libdwfl/dwfl_frame_regs.c   | 12 +++++++++++
 libdwfl/frame_unwind.c      | 40 ++++++++++++++-----------------------
 libdwfl/libdwfl.h           |  6 ++++++
 libdwfl/libdwflP.h          | 10 +++++++---
 libdwfl/linux-core-attach.c |  2 +-
 7 files changed, 44 insertions(+), 29 deletions(-)

diff --git a/NEWS b/NEWS
index 392f2edc..eb1a2c6b 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,8 @@ debuginfod: Add --disable-source-scan option.
 
 libdwfl: Add new function dwfl_get_debuginfod_client.
 
+libdwfl: Add new function dwfl_frame_reg.
+
 Version 0.187
 
 debuginfod: Support -C option for connection thread pooling.
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;
diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
index 83b1abef..a4bd3884 100644
--- a/libdwfl/dwfl_frame_regs.c
+++ b/libdwfl/dwfl_frame_regs.c
@@ -59,3 +59,15 @@ 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)
+
+int
+dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val)
+{
+  int res = __libdwfl_frame_reg_get (state, regno, val);
+  if (res == -1)
+      __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
+  else if (res == 1)
+      __libdwfl_seterrno (DWFL_E_REGISTER_VAL_UNKNOWN);
+  return res;
+}
+INTDEF(dwfl_frame_reg)
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 9ac33833..8185d84b 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -43,21 +43,21 @@
    error.  */
 #define DWARF_EXPR_STEPS_MAX 0x1000
 
-bool
+int
 internal_function
 __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno, Dwarf_Addr *val)
 {
   Ebl *ebl = state->thread->process->ebl;
   if (! ebl_dwarf_to_regno (ebl, &regno))
-    return false;
+    return -1;
   if (regno >= ebl_frame_nregs (ebl))
-    return false;
+    return -1;
   if ((state->regs_set[regno / sizeof (*state->regs_set) / 8]
        & ((uint64_t) 1U << (regno % (sizeof (*state->regs_set) * 8)))) == 0)
-    return false;
+    return 1;
   if (val)
     *val = state->regs[regno];
-  return true;
+  return 0;
 }
 
 bool
@@ -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 (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 (INTUSE (dwfl_frame_reg) (state, op->number, &val1) != 0 || ! 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 (INTUSE (dwfl_frame_reg) (state, op->atom - DW_OP_breg0, &val1) != 0)
 	    {
 	      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 (INTUSE (dwfl_frame_reg) (state, op->number, &val1) != 0)
 	    {
 	      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, &regval))
+	      if (INTUSE (dwfl_frame_reg) (state, regno, &regval) != 0)
 		continue;
 	    }
 	  else
@@ -638,9 +627,10 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
     }
   if (unwound->pc_state == DWFL_FRAME_STATE_ERROR)
     {
-      if (__libdwfl_frame_reg_get (unwound,
-				   frame->fde->cie->return_address_register,
-				   &unwound->pc))
+      int res = INTUSE (dwfl_frame_reg) (unwound,
+          frame->fde->cie->return_address_register,
+          &unwound->pc);
+      if (res == 0)
 	{
 	  /* PPC32 __libc_start_main properly CFI-unwinds PC as zero.
 	     Currently none of the archs supported for unwinding have
@@ -698,7 +688,7 @@ getfunc (int firstreg, unsigned nregs, Dwarf_Word *regs, void *arg)
   Dwfl_Frame *state = arg;
   assert (firstreg >= 0);
   while (nregs--)
-    if (! __libdwfl_frame_reg_get (state, firstreg++, regs++))
+    if (INTUSE (dwfl_frame_reg) (state, firstreg++, regs++) != 0)
       return false;
   return true;
 }
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index b323e8fb..1230972c 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -798,6 +798,12 @@ 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);
 
+/* 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..964daad5 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 (REGISTER_VAL_UNKNOWN, N_("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"))		      \
@@ -277,13 +278,15 @@ struct Dwfl_Frame
 };
 
 /* Fetch value from Dwfl_Frame->regs indexed by DWARF REGNO.
-   No error code is set if the function returns FALSE.  */
-bool __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
+   No error code is set if the function returns 0,
+   -1 on error (invalud DWARF register number),
+   1 if the value of the register in the frame is unknown.  */
+int __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
 			      Dwarf_Addr *val)
   internal_function;
 
 /* Store value to Dwfl_Frame->regs indexed by DWARF REGNO.
-   No error code is set if the function returns FALSE.  */
+   No error code is set if the function returns TRUE.  */
 bool __libdwfl_frame_reg_set (Dwfl_Frame *state, unsigned regno,
 			      Dwarf_Addr val)
   internal_function;
@@ -786,6 +789,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.  */
diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index f68062f0..ee7afa40 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -257,7 +257,7 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
 	     FIXME: It depends now on their order in core notes.
 	     FIXME: It uses private function.  */
 	  if (regno < nregs
-	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL))
+	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL) == 0)
 	    continue;
 	  Dwarf_Word val;
 	  switch (regloc->bits)
-- 
2.37.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] libdw/libdwfl: Add API for accessing registers
  2022-07-29 15:18     ` Di Chen
@ 2022-07-30 23:51       ` Mark Wielaard
  0 siblings, 0 replies; 5+ messages in thread
From: Mark Wielaard @ 2022-07-30 23:51 UTC (permalink / raw)
  To: Di Chen; +Cc: elfutils-devel

[-- Attachment #1: Type: text/plain, Size: 835 bytes --]

Hi,

On Fri, Jul 29, 2022 at 11:18:45PM +0800, Di Chen wrote:
> Re-pushed for fixing run-backtrace-core-sparc.sh failure which resulted
> from some wrong register number.

Thanks. Good the try check caught that copy/paste issue, I would have
probably missed it on review.

> 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.

Thanks, this looks good.  I added ChangeLog entries while
reviewing. Not code issues found. I did change invalud -> invalid in
the comments and tweaked the comment on __libdwfl_frame_reg_get to
explain no error code is ever set. Pushed with those changes.

Cheers,

Mark

[-- Attachment #2: 0001-libdwfl-Add-new-function-dwfl_frame_reg.patch --]
[-- Type: text/x-diff, Size: 10610 bytes --]

From ccc157dc2b96e47d6d1bbb1b066ecbea4975051b Mon Sep 17 00:00:00 2001
From: Di Chen <dichen@redhat.com>
Date: Thu, 28 Jul 2022 16:31:05 +0800
Subject: [PATCH] libdwfl: Add new function dwfl_frame_reg

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 commit adds a new function dwfl_frame_reg to get the value of the
DWARF register number in the given frame.

https://sourceware.org/bugzilla/show_bug.cgi?id=28579

Signed-off-by: Di Chen <dichen@redhat.com>
---
 ChangeLog                   |  4 ++++
 NEWS                        |  1 +
 libdw/ChangeLog             |  4 ++++
 libdw/libdw.map             |  1 +
 libdwfl/ChangeLog           | 15 ++++++++++++++
 libdwfl/dwfl_frame_regs.c   | 12 +++++++++++
 libdwfl/frame_unwind.c      | 40 ++++++++++++++-----------------------
 libdwfl/libdwfl.h           |  6 ++++++
 libdwfl/libdwflP.h          | 10 +++++++---
 libdwfl/linux-core-attach.c |  2 +-
 10 files changed, 66 insertions(+), 29 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index eb2a35f5..0ececcc9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2022-07-28  Di Chen  <dichen@redhat.com>
+
+	* NEWS: Add dwfl_frame_reg.
+
 2022-07-13  Mark Wielaard  <mark@klomp.org>
 
 	* NEWS: Add dwfl_get_debuginfod_client.
diff --git a/NEWS b/NEWS
index 392f2edc..82c86cb6 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,7 @@ Version 0.188 some time after 0.187
 debuginfod: Add --disable-source-scan option.
 
 libdwfl: Add new function dwfl_get_debuginfod_client.
+         Add new function dwfl_frame_reg.
 
 Version 0.187
 
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 6a8f7e51..c9d94e0b 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,7 @@
+2022-07-28  Di Chen  <dichen@redhat.com>
+
+	* libdw.map (ELFUTILS_0.188): Add dwfl_frame_reg.
+
 2022-07-13  Mark Wielaard  <mark@klomp.org>
 
 	* libdw.map (ELFUTILS_0.187): Renamed to...
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;
diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index acdaa013..30f30b14 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,18 @@
+2022-07-28  Di Chen  <dichen@redhat.com>
+
+	* libdwfl.h (dwfl_frame_reg): New function.
+	* libdwflP.h (DWFL_E_REGISTER_VAL_UNKNOWN): New error code.
+	(__libdwfl_frame_reg_get): Return an int.
+	(dwfl_frame_reg): INTDECL.
+	* dwfl_frame_regs.c (dwfl_frame_reg): New function.
+	* frame_unwind.c (__libdwfl_frame_reg_get): Return an int.
+	(state_get_reg): Removed.
+	(expr_eval): Use INTUSE (dwfl_frame_reg) instead of state_get_reg.
+	(handle_cfi): Likewise.
+	(getfunc): Likewise.
+	* linux-core-attach.c (core_set_initial_registers): Check
+	__libdwfl_frame_reg_get returns zero.
+
 2022-07-28  Mark Wielaard  <mark@klomp.org>
 
 	* core-file.c (elf_begin_rand): Replace struct ar_hdr h with
diff --git a/libdwfl/dwfl_frame_regs.c b/libdwfl/dwfl_frame_regs.c
index 83b1abef..a4bd3884 100644
--- a/libdwfl/dwfl_frame_regs.c
+++ b/libdwfl/dwfl_frame_regs.c
@@ -59,3 +59,15 @@ 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)
+
+int
+dwfl_frame_reg (Dwfl_Frame *state, unsigned regno, Dwarf_Word *val)
+{
+  int res = __libdwfl_frame_reg_get (state, regno, val);
+  if (res == -1)
+      __libdwfl_seterrno (DWFL_E_INVALID_REGISTER);
+  else if (res == 1)
+      __libdwfl_seterrno (DWFL_E_REGISTER_VAL_UNKNOWN);
+  return res;
+}
+INTDEF(dwfl_frame_reg)
diff --git a/libdwfl/frame_unwind.c b/libdwfl/frame_unwind.c
index 9ac33833..8185d84b 100644
--- a/libdwfl/frame_unwind.c
+++ b/libdwfl/frame_unwind.c
@@ -43,21 +43,21 @@
    error.  */
 #define DWARF_EXPR_STEPS_MAX 0x1000
 
-bool
+int
 internal_function
 __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno, Dwarf_Addr *val)
 {
   Ebl *ebl = state->thread->process->ebl;
   if (! ebl_dwarf_to_regno (ebl, &regno))
-    return false;
+    return -1;
   if (regno >= ebl_frame_nregs (ebl))
-    return false;
+    return -1;
   if ((state->regs_set[regno / sizeof (*state->regs_set) / 8]
        & ((uint64_t) 1U << (regno % (sizeof (*state->regs_set) * 8)))) == 0)
-    return false;
+    return 1;
   if (val)
     *val = state->regs[regno];
-  return true;
+  return 0;
 }
 
 bool
@@ -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 (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 (INTUSE (dwfl_frame_reg) (state, op->number, &val1) != 0 || ! 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 (INTUSE (dwfl_frame_reg) (state, op->atom - DW_OP_breg0, &val1) != 0)
 	    {
 	      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 (INTUSE (dwfl_frame_reg) (state, op->number, &val1) != 0)
 	    {
 	      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, &regval))
+	      if (INTUSE (dwfl_frame_reg) (state, regno, &regval) != 0)
 		continue;
 	    }
 	  else
@@ -638,9 +627,10 @@ handle_cfi (Dwfl_Frame *state, Dwarf_Addr pc, Dwarf_CFI *cfi, Dwarf_Addr bias)
     }
   if (unwound->pc_state == DWFL_FRAME_STATE_ERROR)
     {
-      if (__libdwfl_frame_reg_get (unwound,
-				   frame->fde->cie->return_address_register,
-				   &unwound->pc))
+      int res = INTUSE (dwfl_frame_reg) (unwound,
+          frame->fde->cie->return_address_register,
+          &unwound->pc);
+      if (res == 0)
 	{
 	  /* PPC32 __libc_start_main properly CFI-unwinds PC as zero.
 	     Currently none of the archs supported for unwinding have
@@ -698,7 +688,7 @@ getfunc (int firstreg, unsigned nregs, Dwarf_Word *regs, void *arg)
   Dwfl_Frame *state = arg;
   assert (firstreg >= 0);
   while (nregs--)
-    if (! __libdwfl_frame_reg_get (state, firstreg++, regs++))
+    if (INTUSE (dwfl_frame_reg) (state, firstreg++, regs++) != 0)
       return false;
   return true;
 }
diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
index b323e8fb..edb537c2 100644
--- a/libdwfl/libdwfl.h
+++ b/libdwfl/libdwfl.h
@@ -798,6 +798,12 @@ 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);
 
+/* Get the value of the DWARF register number in the given frame.
+   Returns zero on success, -1 on error (invalid 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..a2949e74 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 (REGISTER_VAL_UNKNOWN, N_("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"))		      \
@@ -276,9 +277,11 @@ struct Dwfl_Frame
   Dwarf_Addr regs[];
 };
 
-/* Fetch value from Dwfl_Frame->regs indexed by DWARF REGNO.
-   No error code is set if the function returns FALSE.  */
-bool __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
+/* Fetch value from Dwfl_Frame->regs indexed by DWARF REGNO.  The
+   function returns 0 on success, -1 on error (invalid DWARF register
+   number), 1 if the value of the register in the frame is unknown.
+   Even on error, no error code is set.  */
+int __libdwfl_frame_reg_get (Dwfl_Frame *state, unsigned regno,
 			      Dwarf_Addr *val)
   internal_function;
 
@@ -786,6 +789,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.  */
diff --git a/libdwfl/linux-core-attach.c b/libdwfl/linux-core-attach.c
index f68062f0..ee7afa40 100644
--- a/libdwfl/linux-core-attach.c
+++ b/libdwfl/linux-core-attach.c
@@ -257,7 +257,7 @@ core_set_initial_registers (Dwfl_Thread *thread, void *thread_arg_voidp)
 	     FIXME: It depends now on their order in core notes.
 	     FIXME: It uses private function.  */
 	  if (regno < nregs
-	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL))
+	      && __libdwfl_frame_reg_get (thread->unwound, regno, NULL) == 0)
 	    continue;
 	  Dwarf_Word val;
 	  switch (regloc->bits)
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-07-30 23:52 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 14:21 [PATCH v2] libdw/libdwfl: Add API for accessing registers Di Chen
2022-07-21 14:32 ` Mark Wielaard
2022-07-29 13:27   ` Di Chen
2022-07-29 15:18     ` Di Chen
2022-07-30 23:51       ` Mark Wielaard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).