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
