Re: [PATCH][v3] Support .debug_macro

[Petr Machata <pmachata at redhat dot com>]

I'll just post snippets of updated code inline.  The full code is on the
branch.  Since we are discussing relatively minor points, it seems more
effective this way.

Mark Wielaard <mjw@redhat.com> writes:

> 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.

Note that the code only rejects 0xff in .debug_macro.  It serves it
unconditionally in .debug_macinfo.  Whether 0xff is or is not reserved,
dwarf_getmacros shouldn't serve it from .debug_macro.  If it is not made
reserved, and could therefore be legitimately allocated, we will have to
add another interface that does serve it, or at least add an interface
for querying Dwarf macro section version.

>>   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.

I did run the test suite with --enable-valgrind and the macro tests
pass.  There are three failures, run-backtrace-demangle.sh,
run-stack-d-test.sh and run-stack-i-test.sh.  Those fail on master as
well.

>> +	* 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.

Oops.  I sent them separately now.

> A NEWS entry would be nice.

How about this?

diff --git a/NEWS b/NEWS
index aceb3e3..2334646 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,11 @@
+Version 0.161
+
+libdw: dwarf_getmacros will now serve either of .debug_macro and
+       .debug_macinfo transparently.  New interfaces
+       dwarf_getmacros_off, dwarf_macro_getsrcfiles,
+       dwarf_macro_getparamcnt, and dwarf_macro_param are available
+       for more generalized inspection of macros and their parameters.
+
 Version 0.160
 
 libdw: New functions dwarf_cu_getdwarf, dwarf_cu_die.

>
>> +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?

C99.  It says "I want an array with at least 255 elements".  I think I
could just drop the static in this case and just require exactly 255
elements.

>> +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?

Ah, indeed.  I originally thought I'd use it as a function pointer, so
this was for interface compatibility.  But eventually I just made a
static dispatch.

diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index 7f889a9..8d9d78b 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -117,10 +117,7 @@ init_macinfo_table (void)
 }
 
 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)
+get_macinfo_table (Dwarf *dbg, Dwarf_Word macoff, Dwarf_Die *cudie)
 {
   assert (cudie != NULL);
 
@@ -276,7 +273,7 @@ cache_op_table (Dwarf *dbg, int sec_index, Dwarf_Off macoff,
 
   Dwarf_Macro_Op_Table *table = sec_index == IDX_debug_macro
     ? get_table_for_offset (dbg, macoff, startp, endp, cudie)
-    : get_macinfo_table (dbg, macoff, startp, endp, cudie);
+    : get_macinfo_table (dbg, macoff, cudie);
 
   if (table == NULL)
     return NULL;

>> +  /* 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.

diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index 7f889a9..8d9d78b 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -436,13 +433,8 @@ dwarf_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
      -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.  */
+  assert (token <= 0);
 
-  if (token > 0)
-    /* A continuation call from DW_AT_macro_info iteration.  */
-    return macro_info_getmacros_off (dbg, macoff, callback, arg, token, NULL);
-
-  /* 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.

diff --git a/libdw/dwarf_getsrclines.c b/libdw/dwarf_getsrclines.c
index 1f460ec..4bb19c2 100644
--- a/libdw/dwarf_getsrclines.c
+++ b/libdw/dwarf_getsrclines.c
@@ -778,9 +778,6 @@ __libdw_getsrclines (Dwarf *dbg, Dwarf_Off debug_line_offset,
 const char *
 __libdw_getcompdir (Dwarf_Die *cudie)
 {
-  if (cudie == NULL)
-    return NULL;
-
   Dwarf_Attribute compdir_attr_mem;
   Dwarf_Attribute *compdir_attr = INTUSE(dwarf_attr) (cudie,
 						      DW_AT_comp_dir,

>> @@ -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) ?

No, dwarf_macro_param is zero-based.  (As I believe it should be.)
Shall I document this?

-/* Get IDX-th parameter of MACRO, and stores it to *ATTRIBUTE.
-   Returns 0 on success or -1 for errors.
+/* Get IDX-th parameter of MACRO (numbered from zero), and stores it
+   to *ATTRIBUTE.  Returns 0 on success or -1 for errors.

>> +/* 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?

diff --git a/libdw/libdw.h b/libdw/libdw.h
index cd7f5d1..9ac8ea4 100644
--- a/libdw/libdw.h
+++ b/libdw/libdw.h
@@ -831,8 +831,8 @@ extern int dwarf_entry_breakpoints (Dwarf_Die *die, Dwarf_Addr **bkpts);
    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.  Returns -1 for errors or 0 if there are no more
-   macro entries.
+   where it ended.  A TOKEN of 0 starts the iteration.  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.

>> +/* 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?

The original idea was that you can use dwarf_getmacros_die or
dwarf_getmacros_off to obtain a token and then use dwarf_getmacros_off
to continue the iteration.  dwarf_getmacros_die had a Dwarf_Off*
parameter that it would initialize to the offset.  I rephrased the
paragraph.

diff --git a/libdw/libdw.h b/libdw/libdw.h
index cd7f5d1..9ac8ea4 100644
--- a/libdw/libdw.h
+++ b/libdw/libdw.h
@@ -849,10 +849,9 @@ extern ptrdiff_t dwarf_getmacros (Dwarf_Die *cudie,
      __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.
+   unit to iterate through by offset instead of by CU, and always
+   iterates .debug_macro.  This can be used for handling
+   DW_MACRO_GNU_transparent_include's or similar opcodes.
 
    It is not appropriate to obtain macro unit offset by hand from a CU
    DIE and then request iteration through this interface.  The reason

>> +/* 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.

Which is largely not a problem, because they are compatible.  The only
exception is the dreaded 0xff.  How about this:

@@ -876,16 +875,19 @@ extern int dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro,
   __nonnull_attribute__ (2, 3, 4);
 
 /* 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.  */
+   DW_MACINFO_* domain or DW_MACRO_GNU_* domain.  The two domains have
+   compatible values, so it's OK to use either of them for
+   comparisons.  The only differences is 0xff, which currently is
+   never served from .debug_macro, and can thus be safely assumed to
+   mean DW_MACINFO_vendor_ext.  */
 extern int dwarf_macro_opcode (Dwarf_Macro *macro, unsigned int *opcodep)
      __nonnull_attribute__ (2);
 

Thanks,
Petr