Re: [PATCH v3 2/6] libdw: Add dwarf_getalt, dwarf_setalt

[Mark Wielaard <mjw at redhat dot com>]

On Thu, 2014-04-17 at 17:33 +0200, Florian Weimer wrote:
> > Two questions. I think either answer is fine, but it should be
> > explicitly said/documented.
> > 1) Should we verify the underlying ELF files to see whether there is
> > a .gnu_debugaltlink with matching build_id in the alt file? If not then
> > we should document that the user is responsible for the sanity checking.
> 
> I don't think we should.  I think build IDs are optional, and we do not 
> have sufficient information about the file paths to check that they match.

OK agreed. Documentation looks good.

> > 2) Should we allow resetting the alt file once set? If we do, like here,
> > we should add a warning to dwarf_getalt that dwarf_setalt might
> > reset/end the underlying Dwarf.
> 
> But it doesn't—dwarf_setalt does not take ownership.  I tried to make 
> this more clear:
> 
> /* Retrieves the DWARF descriptor for debugaltlink data.  Returns NULL
>     if no alternate debug data has been supplied.  Unless the
>     debugaltlink data was provided by calling dwarf_setalt, a call to
>     dwarf_end (MAIN) will deallocate it, invalidating the returned
>     pointer.  */
> extern Dwarf *dwarf_getalt (Dwarf *main);
> 
> /* Provides the data referenced by the .gnu_debugaltlink section.  The
>     caller should check that MAIN and ALT match (i.e., they have the
>     same build ID).  It is the responsibility of the caller to ensure
>     that the data referenced by ALT stays valid while it is used by
>     MAIN, until dwarf_setalt is called on MAIN with a different
>     descriptor, or dwarf_end.  */
> extern void dwarf_setalt (Dwarf *main, Dwarf *alt);

Aha, I see why I was confused. Sorry. The "problem" isn't here. It is in
patch 5 "Move .gnu_debugaltlink handling from libdw to libdwfl" (I'll
comment in a minute on that one). When the handling is moved the whole
free_alt flag should be removed from the struct Dwarf and put in the
struct Dwfl_Module IMHO. Then it is also clear in the code that libdw
itself never takes ownership itself.

So I think the documentation for dwarf_setalt is fine now. But for
dwarf_getalt I made you add a confusing sentence "Unless..." lets just
remove that.

Could you post an updated patch with that documentation change. The code
is fine, so I want to add it to my branch now.

Thanks,

Mark