Re: [PATCH][v3] Support .debug_macro

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2014-10-17 at 14:15 +0200, Petr Machata wrote:
> This is an updated .debug_macro patch with the following changes, as
> discussed here and on IRC:

Thanks and sorry for the delay in reviewing.

> - .debug_macinfo prototype table is initialized in a ctor.

Looks nicer. Thanks.

> - dwarf_getmacros now serves .debug_macro as well, but in that case
>   refuses to serve 0xff.
>
>   In correlation with this change, I decided to drop dwarf_macro_version
>   and dwarf_getmacros_die.  Currently this just is not necessary at all,
>   because all existing opcodes that have the same value in DW_MACINFO_*
>   and DW_MACRO_GNU_*, have also the same behavior, and 0xff is not
>   allocated in DW_MACRO_GNU_* (and likely never will be, because the
>   allocations will be done under Dwarf 5 standard).
> 
>   If Dwarf 5 comes out with possibly incompatible 0xff, we'll have to
>   publish these interfaces, but it's easier to do that than to retract
>   the new interfaces later if it turns out that they are not necessary.
>   Adding these two interfaces should be a short-order matter, both are
>   few-liners.

I do like to wait a little till the DWARF committee comments on your
request to reserve the 0xff value before going this route. 
Did you hear anything back on your request for that? I am not really
comfortable with returning an error if we do encounter 0xff. It seems a
bit dramatic. It seems the code is ready to return it, but we always
pass false for accept_0xff at the moment.

> - Added a comment to dwarf_getmacros_die clarifying lifetime of
>   Dwarf_Macro pointer passed to the callback.

Thanks.

> - Dwarf_Macro_s simplified -- it now only carries table and attribute
>   pointers and opcode.  Added accessor for determining number of forms,
>   because traversing all the index tables correctly is a pain.

Looks good.

> - Interface dwarf_macro_line_offset was replaced with
>   dwarf_macro_getsrcfiles.  This prompted some changes in
>   dwarf_getsrclines.c, which now exposes functions for loading line
>   units by offset instead of by DIE.

I like this change. The patch makes it look like it changes a lot, but
the actual (ignore whitespace) change to dwarf_getsrclines.c looks
clean.

>   We also need somewhere to keep the DW_AT_stmt_list and DW_AT_comp_dir
>   values, because both are needed for the debug line loader.  That
>   somewhere is naturally the macro opcode table.  That means not all
>   .debug_macinfo tables are the same anymore, and we end up treating
>   them pretty much the same as .debug_macro ones--creating a table per
>   unit, and caching it at Dwarf.

Looks good. I couldn't completely convince myself that nothing leaks,
but I think everything is accounted for and cleaned up. But we better
run some valgrind checks to be sure.

>  libdw/ChangeLog            |   39 ++
>  libdw/Makefile.am          |    8 +-
>  libdw/dwarf_end.c          |    6 +
>  libdw/dwarf_error.c        |    3 +-
>  libdw/dwarf_getmacros.c    |  545 ++++++++++++++++---
>  libdw/dwarf_getsrclines.c  | 1275 +++++++++++++++++++++++---------------------
>  libdw/dwarf_macro_param1.c |    8 +-
>  libdw/dwarf_macro_param2.c |   18 +-
>  libdw/libdw.h              |   76 ++-
>  libdw/libdw.map            |   10 +-
>  libdw/libdwP.h             |   87 ++-
>  11 files changed, 1364 insertions(+), 711 deletions(-)
> 
> diff --git a/libdw/ChangeLog b/libdw/ChangeLog
> index 89b2735..37e8700 100644
> --- a/libdw/ChangeLog
> +++ b/libdw/ChangeLog
> @@ -1,3 +1,42 @@
> +2014-09-10  Petr Machata  <pmachata@redhat.com>
> +
> +	* dwarf_macro_getparamcnt.c: New file.
> +	* dwarf_macro_param.c: New file.
> +	* dwarf_macro_getsrcfiles.c: New file.

These files seem to be missing from the patch.

> +	* Makefile.am (libdw_a_SOURCES): Add the new files.
> +	* libdwP.h (struct files_lines_s): New structure.
> +	(DWARF_E_INVALID_OPCODE): New enumerator.
> +	(struct Dwarf): New fields macro_ops, files_lines.
> +	(Dwarf_Macro_Op_Proto, Dwarf_Macro_Op_Table): New structures for
> +	keeping macro opcode prototypes in.
> +	(Dwarf_Macro_s): Redefine from scratch.
> +	(__libdw_getsrclines, __libdw_getcompdir, libdw_macro_nforms): New
> +	internal interfaces.
> +	* dwarf_error.c (errmsgs): Add a message for
> +	DWARF_E_INVALID_OPCODE.
> +	* dwarf_end.c (dwarf_end): Destroy struct Dwarf.macro_ops and
> +	files_lines.
> +	* libdw.h (dwarf_getmacros_off, dwarf_macro_getparamcnt)
> +	(dwarf_macro_getsrcfiles, dwarf_macro_param): New public
> +	interfaces.
> +	* dwarf_getmacros.c (dwarf_getmacros_off): New function,
> +	(get_offset_from, macro_op_compare, build_table)
> +	(init_macinfo_table, get_macinfo_table, get_table_for_offset)
> +	(cache_op_table, read_macros, gnu_macros_getmacros_off)
> +	(macro_info_getmacros_off, do_dwarf_getmacros_die): New helper
> +	functions.
> +	(dwarf_getmacros): Adjust to dispatch to the new interfaces.
> +	* dwarf_getsrclines.c (read_srclines): New function with guts
> +	taken from dwarf_getsrclines.
> +	(__libdw_getsrclines): Likewise.
> +	(__libdw_getcompdir, files_lines_compare): New functions.
> +	(dwarf_getsrclines): Make it dispatch to the new interfaces.
> +	* dwarf_macro_param1.c (dwarf_macro_param1): Adjust to dispatch to
> +	the new interfaces.
> +	* dwarf_macro_param2.c (dwarf_macro_param2): Likewise.
> +	* libdw.map (ELFUTILS_0.161): New. Add dwarf_getmacros_off,
> +	dwarf_macro_getsrcfiles, dwarf_macro_getparamcnt, dwarf_macro_param.

A NEWS entry would be nice.

> +static void
> +build_table (Dwarf_Macro_Op_Table *table,
> +	     Dwarf_Macro_Op_Proto op_protos[static 255])

I didn't know you could do that static 255, is it a GNU extension?

> +{
> +  unsigned ct = 0;
> +  for (unsigned i = 1; i < 256; ++i)
> +    if (op_protos[i - 1].forms != NULL)
> +      table->table[table->opcodes[i - 1] = ct++] = op_protos[i - 1];
> +    else
> +      table->opcodes[i - 1] = 0xff;
> +}


> +static Dwarf_Macro_Op_Table *
> +get_macinfo_table (Dwarf *dbg, Dwarf_Word macoff,
> +		   __attribute__ ((unused)) const unsigned char *readp,
> +		   __attribute__ ((unused)) const unsigned char *const endp,
> +		   Dwarf_Die *cudie)

It is a static local function, why pass unused arguments?

> +static ptrdiff_t
> +macro_info_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
> +			  int (*callback) (Dwarf_Macro *, void *),
> +			  void *arg, ptrdiff_t token, Dwarf_Die *cudie)
> +{
> +  assert (token >= 0);
> +
> +  return read_macros (dbg, IDX_debug_macinfo, macoff,
> +		      callback, arg, token, true, cudie);
> +}
> +
> +ptrdiff_t
> +dwarf_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
> +		     int (*callback) (Dwarf_Macro *, void *),
> +		     void *arg, ptrdiff_t token)
> +{
> +  if (dbg == NULL)
> +    {
> +      __libdw_seterrno (DWARF_E_NO_DWARF);
> +      return -1;
> +    }
> +
> +  /* We use token values > 0 for iteration through .debug_macinfo and
> +     values < 0 for iteration through .debug_macro.  Return value of
> +     -1 also signifies an error, but that's fine, because .debug_macro
> +     always contains at least three bytes of headers and after
> +     iterating one opcode, we should never see anything above -4.  */
> +
> +  if (token > 0)
> +    /* A continuation call from DW_AT_macro_info iteration.  */
> +    return macro_info_getmacros_off (dbg, macoff, callback, arg, token, NULL);

Do we really want to support this for the public dwarf_getmacros_off
interface? Why not just say that dwarf_getmacros_off is just for
iterating (transparently) through new style macros, not macinfo? I don't
see the use case for this function with old style macinfo macros.

dwarf_getmacros and do_dwarf_getmacros_die already call
macro_info_getmacros_off or gnu_macros_getmacros_off directly, so we
shouldn't get here accidentally for old style macros.

> +  /* Either a DW_AT_GNU_macros continuation, or a fresh start
> +     thereof.  */
> +  return gnu_macros_getmacros_off (dbg, macoff, callback, arg, token, true,
> +				   NULL);
> +}

> +/* Get the compilation directory, if any is set.  */
> +const char *
> +__libdw_getcompdir (Dwarf_Die *cudie)
> +{
> +  if (cudie == NULL)
> +    return NULL;

That seems redundant.
dwarf_attr will return NULL on NULL and so does dwarf_fromstring.

> +  Dwarf_Attribute compdir_attr_mem;
> +  Dwarf_Attribute *compdir_attr = INTUSE(dwarf_attr) (cudie,
> +						      DW_AT_comp_dir,
> +						      &compdir_attr_mem);
> +  return INTUSE(dwarf_formstring) (compdir_attr);
> +}

> diff --git a/libdw/dwarf_macro_param2.c b/libdw/dwarf_macro_param2.c
> index b49e60e..cc902c9 100644
> --- a/libdw/dwarf_macro_param2.c
> +++ b/libdw/dwarf_macro_param2.c
> @@ -1,5 +1,5 @@
>  /* Return second macro parameter.
> -   Copyright (C) 2005 Red Hat, Inc.
> +   Copyright (C) 2005, 2014 Red Hat, Inc.
>     This file is part of elfutils.
>     Written by Ulrich Drepper <drepper@redhat.com>, 2005.
>  
> @@ -40,10 +40,16 @@ dwarf_macro_param2 (Dwarf_Macro *macro, Dwarf_Word *paramp, const char **strp)
>    if (macro == NULL)
>      return -1;
>  
> -  if (paramp != NULL)
> -    *paramp = macro->param2.u;
> -  if (strp != NULL)
> -    *strp = macro->param2.s;
> +  Dwarf_Attribute param;
> +  if (dwarf_macro_param (macro, 1, &param) != 0)
> +    return -1;

Shouldn't this call dwarf_macro_param (macro, 2, &param) ?

> -  return 0;
> +  if (param.form == DW_FORM_string
> +      || param.form == DW_FORM_strp)
> +    {
> +      *strp = dwarf_formstring (&param);
> +      return 0;
> +    }
> +  else
> +    return dwarf_formudata (&param, paramp);
>  }

> diff --git a/libdw/libdw.h b/libdw/libdw.h
> index 196d54a..cd7f5d1 100644
> --- a/libdw/libdw.h
> +++ b/libdw/libdw.h
> @@ -826,26 +826,86 @@ extern int dwarf_func_inline_instances (Dwarf_Die *func,
>  extern int dwarf_entry_breakpoints (Dwarf_Die *die, Dwarf_Addr **bkpts);
>  
> 
> -/* Call callback function for each of the macro information entry for
> -   the CU.  */
> +/* Iterate through the macro unit referenced by CUDIE and call
> +   CALLBACK for each macro information entry.  Keeps iterating while
> +   CALLBACK returns DWARF_CB_OK.  If the callback returns
> +   DWARF_CB_ABORT, it stops iterating and returns a continuation
> +   token, which can be used to restart the iteration at the point
> +   where it ended.

To start the iteration pass 0 as token?

>   Returns -1 for errors or 0 if there are no more
> +   macro entries.
> +
> +   Note that the Dwarf_Macro pointer passed to the callback is only
> +   valid for the duration of the callback invocation.
> +
> +   Note that this interface will refuse to serve opcode 0xff from
> +   .debug_macro sections.  Such opcode is considered invalid and will
> +   cause dwarf_getmacros to return with error.  Note that this should
> +   be no limitation as of now, as DW_MACRO_GNU_* domain doesn't
> +   allocate 0xff.  It is however a theoretical possibility with future
> +   Dwarf standards.  */
>  extern ptrdiff_t dwarf_getmacros (Dwarf_Die *cudie,
>  				  int (*callback) (Dwarf_Macro *, void *),
> -				  void *arg, ptrdiff_t offset)
> -     __nonnull_attribute__ (2);
> +				  void *arg, ptrdiff_t token)
> +     __nonnull_attribute__ (2);
> +
> +/* This is similar in operation to dwarf_getmacros, but selects the
> +   unit to iterate through by offset instead of by CU.  This can be
> +   used for handling DW_MACRO_GNU_transparent_include's or similar
> +   opcodes.  Note that with TOKEN of 0, this will always choose to
> +   iterate through .debug_macro, never .debug_macinfo.

This is IMHO slightly confusing (see above). How would you get a
continuation token that is for .debug_macinfo?

> +   It is not appropriate to obtain macro unit offset by hand from a CU
> +   DIE and then request iteration through this interface.  The reason
> +   for this is that if a dwarf_macro_getsrcfiles is later called,
> +   there would be no way to figure out what DW_AT_comp_dir was present
> +   on the CU DIE, and file names referenced in either the macro unit
> +   itself, or the .debug_line unit that it references, might be wrong.
> +   Use dwarf_getmacro.  */
> +extern ptrdiff_t dwarf_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
> +				      int (*callback) (Dwarf_Macro *, void *),
> +				      void *arg, ptrdiff_t token)
> +  __nonnull_attribute__ (3);

> +/* Return macro opcode.  That's a constant that can be either from
> +   DW_MACINFO_* domain if version of MACRO is 0, or from
> +   DW_MACRO_GNU_* domain if the version is 4.  */

Note, we don't have an interface to query the version anymore, so the
user wouldn't know which one it is.

>  extern int dwarf_macro_opcode (Dwarf_Macro *macro, unsigned int *opcodep)
>       __nonnull_attribute__ (2);

Thanks,

Mark