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 <[email protected]> > + > + * 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 <[email protected]>, 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, ¶m) != 0) > + return -1; Shouldn't this call dwarf_macro_param (macro, 2, ¶m) ? > - return 0; > + if (param.form == DW_FORM_string > + || param.form == DW_FORM_strp) > + { > + *strp = dwarf_formstring (¶m); > + return 0; > + } > + else > + return dwarf_formudata (¶m, 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
