[PATCH 03/10 v3] lib: Add eu_tsearch, eu_tfind, eu_tdelete and eu_tdestroy

[Mark Wielaard]

Hi Aaron,

On Fri, Aug 02, 2024 at 07:38:02PM -0400, Aaron Merey wrote:
> From: Heather McIntyre <hsm2@rice.edu>
> 
> Add struct search_tree to hold tree root and lock.  Add new eu_t*
> functions for ensuring synchronized tree access.
> 
> Replace tsearch, tfind, etc with eu_t* equivalents.
> 
> lib:
> 	* Makefile.am (libeu_a_SOURCES): Add eu-search.c.
> 	(noinst_HEADERS): Add eu-search.h and locks.h.
> 	* eu-config.h: Move rwlock macros to locks.h.
> 	* eu-search.c: New file containing tree search functions with
> 	  locking.
> 	* eu-search.h: New file.
> 	* locks.h: New file containing rwlock macros previously in
> 	  eu-config.h.
> libdw:
> 	* cfi.h (struct Dwarf_CFI_s): Change type of search tree members
> 	  from void * to search_tree.
> 	* cie.c: Replace tree search functions with eu-search equivalents.
> 	* dwarf_begin_elf.c (valid_p): Initialize search trees.
> 	* dwarf_end.c (cu_free): Replace tree search functions
> 	  with eu-search equivalents.
> 	* dwarf_getcfi.c (dwarf_getcfi): Initialize search trees.
> 	* dwarf_getlocations.c: Replace search tree functions with
> 	  eu-search equivalents.
> 	  (__libdw_intern_expression): Change type of cache parameter to
> 	  search_tree *.
> 	* dwarf_getmacros.c: Replace tree search functions with
> 	  eu-search equivalents.
> 	* dwarf_getsrclines.c: Ditto.
> 	* fde.c: Ditto.
> 	* frame-cache.c (__libdw_destroy_frame_cache): Initialize search
> 	  trees.
> 	* libdwP.h (struct Dwarf): Change type of search tree members
> 	  from void * to search_tree.
> 	  (struct Dwarf_CU): Ditto.
> 	  (__libdw_intern_expression): Change type of cache parameter to
> 	  search_tree *.
> 	* libdw_find_split_unit.c: Replace tree search functions with
> 	  eu-search equivalents.
> 	* libdw_findcu.c: Ditto.
> libdwfl:
> 	* cu.c: Ditto.
> 	* libdwflP.h (struct Dwfl_Module): Replace void *lazy_cu_root
> 	  with search_tree lazy_cu_tree.
> libelf:
> 	* elf_begin.c (file_read_elf): Initialize rawchunck_tree.
> 	* elf_end.c (elf_end): Replace tree search function with
> 	  eu-search equivalent.
> 	* elf_getdata_rawchunck.c: Ditto.
> 	* libelfP.h (struct Elf): Replace void * rawchuncks memeber with
> 	search_tree rawchunk_tree.

Nice Changelog. (note the memeber typo)

> Signed-off-by: Heather S. McIntyre <hsm2@rice.edu>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> Signed-off-by: Mark Wielaard <mark@klomp.org>
> 
> ---
> 
> v3 changes:
> 
> Moved most locking code to another patch in this series.
> include "eu-search.h" and "locks.h" instead of <eu-search.h>
> and <locks.h>

Thanks, sorry for the extra "style" work.

> On Fri, Jul 19, 2024 at 8:51 AM Mark Wielaard <mark@klomp.org> wrote:
> > Is eu_tdestroy ever used now?
> > It looks like everything now uses eu_search_tree_fine.
> > Could still be a useful function to have, just checking.
> 
> Yes the function less_lazy in libdwfl/cu.c uses it.

Ack. I see I actually mentioned that myself below. Doh.

> > > +#define LOCKS_H     1
> > > +
> > > +#ifdef USE_LOCKS
> > > +# include <pthread.h>
> > > +# include <assert.h>
> >
> > Why the assert.h include?
> 
> It's needed for assert_perror in the RWLOCK_CALL macro

Interesting. That is existing code. So ok.

> > >    /* We know about all the CUs now, we don't need this table.  */
> > > -  tdestroy (mod->lazy_cu_root, nofree);
> > > -  mod->lazy_cu_root = NULL;
> > > +  eu_tdestroy (&mod->lazy_cu_tree, nofree);
> > >  }
> >
> > OK, a eu_tdestroy here, and then there is a eu_search_tree_fini in
> > __libdwfl_module_free, which will call eu_tdestroy again (on the now
> > NULL tree). Is that correct? Does [eu_]tdestroy handle NULL trees?
> 
> Yes NULL is a valid (empty) tree.

Good.

And it looks like you also got rid of the extra dwarf,abbrev,split_locks.

Looks good to go to me.

Thanks,

Mark