This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH v2 2/3] libdw: Add dwarf_set_alt
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 10 Apr 2014 20:11:58 +0200
- Subject: 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