Re: [PATCH] Do not reject type units in dwarf_getsrcfiles and dwarf_getsrclines

[Mark Wielaard <mjw at redhat dot com>]

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