This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2 2/3] libdw: Add dwarf_set_alt


Hi Florian,

On Wed, 2014-04-09 at 16:44 +0200, Florian Weimer wrote:
> +	* libdw.h (dwarf_set_alt): New function.
> +	* libdwP.h (dwarf_set_alt): Internal declaration.
> +	* Makefile.am (libdw_a_SOURCES): Add dwarf_set_alt.c.
> +	* libdw.map (ELFUTILS_0.159): Export dwarf_set_alt.
> +	* dwarf_set_alt.c: New file.
> +	* dwarf_begin_elf.c (try_debugaltlink): Call dwarf_set_alt.
> +	* dwarf_end.c (dwarf_end): Likewise.

Lets start with the bikeshed/nitpick. I would like it to be called
dwarf_setalt (paired with a dwarf_getalt function later) because that
seems more in line with other dwarf_xxxyyy naming. So drop the second
underscore.

> +	  Elf *elf = alt_dwarf->elf;
> +	  if (__check_build_id (alt_dwarf, build_id, id_len) == 0
>  	      && elf_cntl (elf, ELF_C_FDREAD) == 0)
>  	    {
>  	      close (fd);
> -	      result->free_alt = 1;
> +	      INTUSE (dwarf_set_alt) (result, alt_dwarf, true);
>  	      return result;
>  	    }

See below for the result->free_alt = 1 change (I think the flag should
be set here, not in dwarf_setalt).

> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index e65314a..d0b0332 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -112,8 +112,7 @@ dwarf_end (dwarf)
>  	elf_end (dwarf->elf);
>  
>        /* Free the alternative Dwarf descriptor if necessary.  */
> -      if (dwarf->free_alt)
> -	INTUSE (dwarf_end) (dwarf->alt_dwarf);
> +      INTUSE (dwarf_set_alt) (dwarf, NULL, false);

And here I would also keep the original way.

> --- /dev/null
> +++ b/libdw/dwarf_set_alt.c
> @@ -0,0 +1,44 @@
> +/* Create descriptor from ELF descriptor for processing file.

Try a less copy/pasty looking sentence :)

> +/* Provides the data referenced by the .gnu_debugaltlink section.  It
> +   is the responsibility of the caller not ensure that the data
> +   referenced by ALT stays valid as long as MAIN is used.  If
> +   ALT_FREE, ALT will automatically be deallocated by the next call to
> +   dwarf_set_alt or dwarf_end on MAIN.  */
> +extern void dwarf_set_alt (Dwarf *main, Dwarf *alt, bool alt_free);

I don't think we want to expose an alt_free flag and make ownership
rules more complicated. Lets just keep that flag internal. If someone
provides a Dwarf then they own it, if we create it internally then we
own it.

Thanks,

Mark


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]