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 pmachata@redhat.com
- 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