Hi Petr, Looks good. The explicit checking against some TAGs was indeed fragile and not really checking what we wanted. Some small nitpicks below.
On Wed, 2015-04-01 at 22:05 +0200, Petr Machata wrote: > +2015-04-01 Petr Machata <[email protected]> > + > + * libdwP.h (DWARF_E_NOT_CUDIE): New enumerator. > + (is_cudie): New function. > + * dwarf_error.c (errmsgs): Add message for DWARF_E_NOT_CUDIE. > + * dwarf_getsrcfiles.c (dwarf_getsrcfiles): Call is_cudie instead > + of white-listing valid tags. > + * dwarf_getsrclines.c (dwarf_getsrclines.c): Likewise. Note that there is an extra .c in the last line. Should be "(dwarf_getsrclines)". > --- a/libdw/dwarf_getsrcfiles.c > +++ b/libdw/dwarf_getsrcfiles.c > @@ -39,10 +39,11 @@ > int > dwarf_getsrcfiles (Dwarf_Die *cudie, Dwarf_Files **files, size_t *nfiles) > { > - if (unlikely (cudie == NULL > - || (INTUSE(dwarf_tag) (cudie) != DW_TAG_compile_unit > - && INTUSE(dwarf_tag) (cudie) != DW_TAG_partial_unit))) > - return -1; > + if (cudie == NULL || ! is_cudie (cudie)) > + { > + __libdw_seterrno (DWARF_E_NOT_CUDIE); > + return -1; > + } The convention in most libdw functions is to signal error on NULL input (return -1), but not set dw errno. That assumes people "chain" function calls when getting a DIE and using it. When calling a function to get a DIE pointer NULL is returned and dw errno set on error. So that a next call that sees a NULL DIE knows to bail out early, return -1, but not also sets dw errno. The caller can then just get the original dw errno from the last call that failed. So I would split this in two checks. > int > dwarf_getsrclines (Dwarf_Die *cudie, Dwarf_Lines **lines, size_t *nlines) > { > - if (unlikely (cudie == NULL > - || (INTUSE(dwarf_tag) (cudie) != DW_TAG_compile_unit > - && INTUSE(dwarf_tag) (cudie) != DW_TAG_partial_unit))) > - return -1; > + if (cudie == NULL || ! is_cudie (cudie)) > + { > + __libdw_seterrno (DWARF_E_NOT_CUDIE); > + return -1; > + } Likewise. OK, with those changes. Thanks, Mark
