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