Re: patch 0/2 debuginfod submission

[Mark Wielaard]

Hi Frank,

On Wed, 2019-10-30 at 14:11 -0400, Frank Ch. Eigler wrote:
> Revised code to look like this (libdwfl/find-debuginfo.c):
> 
> 00404 /* NB: this is slightly thread-unsafe */
> 00405 static __typeof__ (debuginfod_find_debuginfo) 
> *fp_debuginfod_find_debuginfo;
> 00406
> 00407 if (fp_debuginfod_find_debuginfo == NULL)
> 00408   {
> 00409 void *debuginfod_so = dlopen("libdebuginfod-" VERSION ".so", 
> RTLD_LAZY);
> 00410 if (debuginfod_so == NULL)
> 00411   debuginfod_so = dlopen("libdebuginfod.so", RTLD_LAZY);
> 00412 if (debuginfod_so != NULL)
> 00413   fp_debuginfod_find_debuginfo = dlsym (debuginfod_so, 
> "debuginfod_find_debuginfo");
> 00414 if (fp_debuginfod_find_debuginfo == NULL)
> 00415   fp_debuginfod_find_debuginfo = (void *) -1; /* never try 
> again */
> 00416   }
> 00417
> 00418 if (fp_debuginfod_find_debuginfo != NULL && 
> fp_debuginfod_find_debuginfo != (void *) -1)
> 00419   {
> 00420 /* If all else fails and a build-id is available, query the
> 00421debuginfo-server if enabled.  */
> 00422 if (fd < 0 && bits_len > 0)
> 00423   fd = (*fp_debuginfod_find_debuginfo) (bits, bits_len, NULL);
> 00424   }

Thanks, that looks perfectly fine for now. This should also be not too
hard to make thread-safe when using atomics.

> > > We document returning standard errnos generally, and a few particular
> > > ones for network unreachability.
> > 
> > But those aren't propagated to users for libdwfl find_elf or
> > find_debuginfo. I was really just wondering if you thought about that
> > in the scope of the libdwfl calls. Currently there is no real good way
> > to do any error reporting there. So it isn't a big concern. But if you
> > have any ideas for improvement that would be nice.
> 
> Will think about it.

One idea is described here:
https://lists.fedorahosted.org/pipermail/elfutils-devel/2015-May/thread.html#4841

Cheers,

Mark

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Mark Wielaard]

On Wed, 2019-10-30 at 16:49 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> > Also, if we would adopt dwfl_attach_thread then I think it should take
> > a Dwfl_Thread object not a pid/tid as argument.
> 
> In that case, we'd probably want to expose the internal getthread
> function with something like:
> 
> /* Find the thread with the given thread ID.  Returns zero if the
>thread was processed, -1 on error (including when no thread with the
>given thread ID exists), or the return value of the callback when not
>DWARF_CB_OK.  */
> int dwfl_getthread (Dwfl *dwfl, pid_t tid,
>   int (*callback) (Dwfl_Thread *thread, void *arg),
>   void *arg)
>   __nonnull_attribute__ (1, 3);
> 
> Then dwfl_attach_thread could be used with either dwfl_getthread or
> dwfl_getthreads, which is nice. However, a crucial part of this
> feature
> is being able to access the Dwfl_Thread outside of the callback. 
>
> Since
> the Dwfl_Thread is currently on the stack, I see a couple of options:
> 
> 1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread()
> return
>a copy (like it does in this patch).
> 2. We always allocate the Dwfl_Thread on the heap and free it before
>returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
>called. If it was, it will be freed by dwfl_detach_thread()
> instead.
> 
> Option 2 sounds better to me. What do you think?

To be honest I am not sure I like either.

I think it is mainly because this isn't really about
attaching/detaching from a thread but stopping/resuming it. That is
part of what set_initial_registers does. So what
dwfl_attach_thread/dwfl_detach_thread really do is setting up the
Dwfl_Thread so it can be queried/used.

You are attached/detached on the process as a whole. Which is done with
the dwfl_linux_proc_attach call.

So basically I think what we want is an interface which you call
pauzing the thread.

Sorry for not having a clear vision of the perfect interface yet.

Cheers,

Mark

Re: [PATCH 4/5] libdwfl: cache Dwfl_Module and Dwarf_Frame for Dwfl_Frame

[Mark Wielaard]

On Wed, 2019-10-30 at 16:55 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 02:04:42PM +0100, Mark Wielaard wrote:
> > Hi Omar,
> > 
> > On Mon, 2019-10-07 at 02:05 -0700, Omar Sandoval wrote:
> > > The next change will need to have the Dwarf_Frame readily available, so
> > > rather than finding it again every time, let's cache it for reuse. The
> > > CFI frame can also be useful to clients of libdwfl, so add
> > > dwfl_frame_dwarf_frame to get it. Similarly, the Dwfl_Module is also
> > > frequently needed in conjunction with the frame, so cache it and add
> > > dwfl_frame_module.
> > 
> > I can see how this is useful. But it seems the Dwarf_Frame is only
> > cached when __libdwfl_frame_unwind is called. Which I believe isn't
> > done for the initial frame. Also it isn't clear how to propagate any
> > errors when NULL is returned. Maybe dwfl_frame_dwarf_frame () should
> > check first to see if frame is NULL and then call lookup the CFI and
> > call dwarf_cfi_addrframe if not?
> 
> Yes, that makes sense. Rather than doing the lookups in
> __libdwfl_frame_unwind and handle_cfi, I can move the lookups to
> dwfl_frame_module and dwfl_frame_dwarf_frame, have those functions cache
> it internally, and make __libdwfl_frame_unwind and handle_cfi call those
> functions.

dwfl_frame_dwarf_frame should also save any errors, so that when it is
called again it can set it again when returning NULL. Like what
dwfl_module_getdwarf does.

Thanks,

Mark

Re: [PATCH 5/5] libdwfl: add interface for evaluating DWARF expressions in a frame

[Mark Wielaard]

On Wed, 2019-10-30 at 16:59 -0700, Omar Sandoval wrote:
> On Wed, Oct 30, 2019 at 02:23:06PM +0100, Mark Wielaard wrote:
> > Having some examples/testcases would also show how/where to get the
> > DWARF expressions to use with this new function.
> 
> Sounds good, I'll put some examples/test cases together. FWIW, I'm using
> it in drgn to get register values like so:
> 
> LIBDRGN_PUBLIC struct drgn_error *
> drgn_stack_frame_register(struct drgn_stack_frame frame,
> enum drgn_register_number regno, uint64_t *ret)
> {
>   const Dwarf_Op op = { .atom = DW_OP_regx, .number = regno, };
>   Dwarf_Addr value;
> 
>   if (!dwfl_frame_eval_expr(frame.trace->frames[frame.i], &op, 1, &value))
>   return drgn_error_libdwfl();
>   *ret = value;
>   return NULL;
> }

O! I hadn't even thought of that. Funny using it with "hand
constructed" DWARF expressions.

> Later, I plan to use this for location expressions for local variables
> that I get out of DWARF.

Yes, that was what I assumed you were using it for already.
I think it will work for that. But it would be nice to have some
code/example that actually does it.

Thanks,

Mark

Re: [PATCH 3/5] libdwfl: add interface for attaching to/detaching from threads

[Omar Sandoval]

On Thu, Oct 31, 2019 at 05:21:17PM +0100, Mark Wielaard wrote:
> On Wed, 2019-10-30 at 16:49 -0700, Omar Sandoval wrote:
> > On Wed, Oct 30, 2019 at 01:47:52PM +0100, Mark Wielaard wrote:
> > > Also, if we would adopt dwfl_attach_thread then I think it should take
> > > a Dwfl_Thread object not a pid/tid as argument.
> > 
> > In that case, we'd probably want to expose the internal getthread
> > function with something like:
> > 
> > /* Find the thread with the given thread ID.  Returns zero if the
> >thread was processed, -1 on error (including when no thread with the
> >given thread ID exists), or the return value of the callback when not
> >DWARF_CB_OK.  */
> > int dwfl_getthread (Dwfl *dwfl, pid_t tid,
> > int (*callback) (Dwfl_Thread *thread, void *arg),
> > void *arg)
> >   __nonnull_attribute__ (1, 3);
> > 
> > Then dwfl_attach_thread could be used with either dwfl_getthread or
> > dwfl_getthreads, which is nice. However, a crucial part of this
> > feature
> > is being able to access the Dwfl_Thread outside of the callback. 
> >
> > Since
> > the Dwfl_Thread is currently on the stack, I see a couple of options:
> > 
> > 1. We keep Dwfl_Thread on the stack and make dwfl_attach_thread()
> > return
> >a copy (like it does in this patch).
> > 2. We always allocate the Dwfl_Thread on the heap and free it before
> >returning from dwfl_getthread(s) _unless_ dwfl_attach_thread() was
> >called. If it was, it will be freed by dwfl_detach_thread()
> > instead.
> > 
> > Option 2 sounds better to me. What do you think?
> 
> To be honest I am not sure I like either.
> 
> I think it is mainly because this isn't really about
> attaching/detaching from a thread but stopping/resuming it. That is
> part of what set_initial_registers does. So what
> dwfl_attach_thread/dwfl_detach_thread really do is setting up the
> Dwfl_Thread so it can be queried/used.
> 
> You are attached/detached on the process as a whole. Which is done with
> the dwfl_linux_proc_attach call.
> 
> So basically I think what we want is an interface which you call
> pauzing the thread.
> 
> Sorry for not having a clear vision of the perfect interface yet.

No problem, I'm sure we'll be able to come up with something better :)

I was hesitant to call this dwfl_stop_thread/dwfl_resume_thread because
you aren't necessarily stopping/resuming anything, for example when
you're working with a core dump. Plus, the detach terminology is
consistent with Dwfl_Thread_Callbacks::thread_detach.

But, stop/resume is more descriptive. Does that sound better to you? The
semantics could be that a Dwfl_Thread is valid after dwfl_getthread(s)
returns if and only if it is currently paused. The Dwfl_Thread is freed
on dwfl_resume_thread() or when dwfl_getthread(s) returns, whichever
comes last.

Thanks,
Omar

Re: [PATCH] libdw: Don't free uninitialized Dwarf_Abbrev_Hash's of "fake" CUs.

[Mark Wielaard]

Hi Jonathon,

On Wed, Oct 30, 2019 at 09:29:02PM -0500, Jonathon Anderson wrote:
> fake_{loc,loclists,addr}_cu are Dwarf_CUs that are created separate from
> all the others, so their contents are minimal and mostly initialized by
> a calloc. On dwarf_end however, they are freed through the same code path
> as all the others, so they call DAH_free like all the others. This changes
> that so that these three are exempt from DAH and split-DWARF matters, and
> swaps the calloc for a malloc so Memcheck will catch any others.

Thanks for catching that!

> ---
> Sorry to keep adding patches, but ran across this while tinkering about.
>
> Its not a current issue, but when the concurrent hash table is
> integrated this could cause some issues (on some system somewhere,
> maybe). In that case there are enough moving internals that a calloc
> may not initialize it properly.
>
> Its also an error waiting to happen, so this is this just cleans it
> up with some semblance of logical correctness.

Yes, this is very good. But... see below.

> (In other news, I finally figured out why git send-email wasn't
> working for me!  Thanks for everyone's patience, especially
> Wielaard's :) )

Works perfectly.

> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index 8d137414..1b2c5a45 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -223,7 +223,7 @@ valid_p (Dwarf *result)
>   inside the .debug_loc or .debug_loclists section.  */
>if (result != NULL && result->sectiondata[IDX_debug_loc] != NULL)
>  {
> -  result->fake_loc_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +  result->fake_loc_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>if (unlikely (result->fake_loc_cu == NULL))
>   {
> Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -240,12 +240,15 @@ valid_p (Dwarf *result)
> result->fake_loc_cu->endp
>   = (result->sectiondata[IDX_debug_loc]->d_buf
>  + result->sectiondata[IDX_debug_loc]->d_size);
> +   result->fake_loc_cu->locs = NULL;
> +   result->fake_loc_cu->address_size = 0;
> +   result->fake_loc_cu->version = 0;
>   }
>  }
>  
>if (result != NULL && result->sectiondata[IDX_debug_loclists] != NULL)
>  {
> -  result->fake_loclists_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +  result->fake_loclists_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>if (unlikely (result->fake_loclists_cu == NULL))
>   {
> Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -263,6 +266,9 @@ valid_p (Dwarf *result)
> result->fake_loclists_cu->endp
>   = (result->sectiondata[IDX_debug_loclists]->d_buf
>  + result->sectiondata[IDX_debug_loclists]->d_size);
> +   result->fake_loclists_cu->locs = NULL;
> +   result->fake_loclists_cu->address_size = 0;
> +   result->fake_loclists_cu->version = 0;
>   }
>  }
>  
> @@ -272,7 +278,7 @@ valid_p (Dwarf *result)
>   inside the .debug_addr section, if it exists.  */
>if (result != NULL && result->sectiondata[IDX_debug_addr] != NULL)
>  {
> -  result->fake_addr_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
> +  result->fake_addr_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
>if (unlikely (result->fake_addr_cu == NULL))
>   {
> Dwarf_Sig8_Hash_free (&result->sig8_hash);
> @@ -291,6 +297,9 @@ valid_p (Dwarf *result)
> result->fake_addr_cu->endp
>   = (result->sectiondata[IDX_debug_addr]->d_buf
>  + result->sectiondata[IDX_debug_addr]->d_size);
> +   result->fake_addr_cu->locs = NULL;
> +   result->fake_addr_cu->address_size = 0;
> +   result->fake_addr_cu->version = 0;
>   }
>  }

This all looks correct.

> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index a2e94436..a32a149e 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -52,18 +52,22 @@ cu_free (void *arg)
>  {
>struct Dwarf_CU *p = (struct Dwarf_CU *) arg;
>  
> -  Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
> -
>tdestroy (p->locs, noop_free);
>  
> -  /* Free split dwarf one way (from skeleton to split).  */
> -  if (p->unit_type == DW_UT_skeleton
> -  && p->split != NULL && p->split != (void *)-1)
> +  if(p != p->dbg->fake_loc_cu && p != p->dbg->fake_loclists_cu
> + && p != p->dbg->fake_addr_cu)
>  {
> -  /* The fake_addr_cu might be shared, only release one.  */
> -  if (p->dbg->fake_addr_cu == p->split->dbg->fake_addr_cu)
> - p->split->dbg->fake_addr_cu = NULL;
> -  INTUSE(dwarf_end) (p->split->dbg);
> +  Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
> +
> +  /* Free split dwarf one way (from skeleton to split).  */
> +  if (p->unit_type == DW_UT_skeleton
> +   && p->split != NULL && p->split != (void *)-1)
> + {
> +  /* The fake_addr_cu might be shared, only release one.  */
> +  if (p->dbg->fake_addr_cu == p->split->dbg->fake_addr_cu)
> + p->split-

[PATCH] libdw: Don't free uninitialized Dwarf_Abbrev_Hash's of "fake" CUs.

[Jonathon Anderson]

fake_{loc,loclists,addr}_cu are Dwarf_CUs that are created separate from
all the others, so their contents are minimal and mostly initialized by
a calloc. On dwarf_end however, they are freed through the same code path
as all the others, so they call DAH_free like all the others. This changes
that so that these three are exempt from DAH and split-DWARF matters, and
swaps the calloc for a malloc so Memcheck will catch any others.
---
> The Dwarf_Abbrev_Hash_free looks in the correct place.  But I don't
> believe the Free split dwarf hunk should not be under the same if
> not-fake block.

That can be fixed.

I don't really like `unit_type = 0`, but its the only value that isn't one
of the DW_UT values. And its what the calloc would've done.

 libdw/dwarf_begin_elf.c | 21 ++---
 libdw/dwarf_end.c   |  9 +++--
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
index 8d137414..ebb3eaea 100644
--- a/libdw/dwarf_begin_elf.c
+++ b/libdw/dwarf_begin_elf.c
@@ -223,7 +223,7 @@ valid_p (Dwarf *result)
  inside the .debug_loc or .debug_loclists section.  */
   if (result != NULL && result->sectiondata[IDX_debug_loc] != NULL)
 {
-  result->fake_loc_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
+  result->fake_loc_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
   if (unlikely (result->fake_loc_cu == NULL))
{
  Dwarf_Sig8_Hash_free (&result->sig8_hash);
@@ -240,12 +240,17 @@ valid_p (Dwarf *result)
  result->fake_loc_cu->endp
= (result->sectiondata[IDX_debug_loc]->d_buf
   + result->sectiondata[IDX_debug_loc]->d_size);
+ result->fake_loc_cu->locs = NULL;
+ result->fake_loc_cu->address_size = 0;
+ result->fake_loc_cu->version = 0;
+ result->fake_loc_cu->unit_type = 0;
+ result->fake_loc_cu->split = NULL;
}
 }
 
   if (result != NULL && result->sectiondata[IDX_debug_loclists] != NULL)
 {
-  result->fake_loclists_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
+  result->fake_loclists_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
   if (unlikely (result->fake_loclists_cu == NULL))
{
  Dwarf_Sig8_Hash_free (&result->sig8_hash);
@@ -263,6 +268,11 @@ valid_p (Dwarf *result)
  result->fake_loclists_cu->endp
= (result->sectiondata[IDX_debug_loclists]->d_buf
   + result->sectiondata[IDX_debug_loclists]->d_size);
+ result->fake_loclists_cu->locs = NULL;
+ result->fake_loclists_cu->address_size = 0;
+ result->fake_loclists_cu->version = 0;
+ result->fake_loclists_cu->unit_type = 0;
+ result->fake_loclists_cu->split = NULL;
}
 }
 
@@ -272,7 +282,7 @@ valid_p (Dwarf *result)
  inside the .debug_addr section, if it exists.  */
   if (result != NULL && result->sectiondata[IDX_debug_addr] != NULL)
 {
-  result->fake_addr_cu = (Dwarf_CU *) calloc (1, sizeof (Dwarf_CU));
+  result->fake_addr_cu = (Dwarf_CU *) malloc (sizeof (Dwarf_CU));
   if (unlikely (result->fake_addr_cu == NULL))
{
  Dwarf_Sig8_Hash_free (&result->sig8_hash);
@@ -291,6 +301,11 @@ valid_p (Dwarf *result)
  result->fake_addr_cu->endp
= (result->sectiondata[IDX_debug_addr]->d_buf
   + result->sectiondata[IDX_debug_addr]->d_size);
+ result->fake_addr_cu->locs = NULL;
+ result->fake_addr_cu->address_size = 0;
+ result->fake_addr_cu->version = 0;
+ result->fake_addr_cu->unit_type = 0;
+ result->fake_addr_cu->split = NULL;
}
 }
 
diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
index a2e94436..265239c4 100644
--- a/libdw/dwarf_end.c
+++ b/libdw/dwarf_end.c
@@ -52,8 +52,6 @@ cu_free (void *arg)
 {
   struct Dwarf_CU *p = (struct Dwarf_CU *) arg;
 
-  Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
-
   tdestroy (p->locs, noop_free);
 
   /* Free split dwarf one way (from skeleton to split).  */
@@ -65,6 +63,13 @@ cu_free (void *arg)
p->split->dbg->fake_addr_cu = NULL;
   INTUSE(dwarf_end) (p->split->dbg);
 }
+
+  /* Only free the Abbrev_Hash if its not a fake CU.  */
+  if(p != p->dbg->fake_loc_cu && p != p->dbg->fake_loclists_cu
+ && p != p->dbg->fake_addr_cu)
+{
+  Dwarf_Abbrev_Hash_free (&p->abbrev_hash);
+}
 }
 
 
-- 
2.24.0.rc2