Re: [PATCH][v3] Support .debug_macro

[Petr Machata]

Mark Wielaard <m...@redhat.com> writes:

> If it isn't reserved then we should indeed add a dwarf_macro_version ()
> function. But I hope we can just keep one function to iterate the
> macros. Can we use "1" as special token, instead of "0" for
> dwarf_getmacros () to start "new style capable users" iteration? Then we
> have a way to know whether the user "promises" to use
> dwarf_macro_getparamcnt and dwarf_macro_param instead of old style
> accessors.

1 is a valid offset in .debug_macinfo.  I think that a named constant
that evaluates to PTRDIFF_MAX would work.  Together with a version-query
interface, this would work well for supporting the dual nature of 0xff.

>> > 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.
>
> Note that the testsuite doesn't check leaking, just that the calgrind
> memcheck tool doesn't produce errors. We could try to add
> --leak-check=yes, but I don't think all tests are really leak free.
> Maybe we should make them so.

Ah, I thought it did.  In that case:

$ make valgrind_cmd="'valgrind --leak-check=full --log-file=$(pwd)/out'" -j 4 
check TESTS=run-dwarf-getmacros.sh
[...]
PASS: run-dwarf-getmacros.sh
=============
1 test passed
=============

$ cat out
[...]
==29009==    definitely lost: 4,320 bytes in 1 blocks

Oopsie.  Luckily that was juts the test itself neglecting to end the
test Dwarf:

diff --git a/tests/dwarf-getmacros.c b/tests/dwarf-getmacros.c
index 588dd75..f203d5b 100644
--- a/tests/dwarf-getmacros.c
+++ b/tests/dwarf-getmacros.c
@@ -123,5 +123,7 @@ main (int argc __attribute__ ((unused)), char *argv[])
        break;
       }

+  dwarf_end (dbg);
+
   return 0;
 }

$ make valgrind_cmd="'valgrind --leak-check=full --log-file=$(pwd)/out'" -j 4 
check TESTS=run-dwarf-getmacros.sh
[...]
PASS: run-dwarf-getmacros.sh
=============
1 test passed
=============

$ cat out
[...]
==29398==     in use at exit: 0 bytes in 0 blocks

>> >> @@ -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.)
>
> OK, but then shouldn't dwarf_macro_param1 use 0 instead of 1 as idx?

Uhh, yes it should!

diff --git a/libdw/dwarf_macro_param1.c b/libdw/dwarf_macro_param1.c
index 337ad6f..87ce003 100644
--- a/libdw/dwarf_macro_param1.c
+++ b/libdw/dwarf_macro_param1.c
@@ -41,7 +41,7 @@ dwarf_macro_param1 (Dwarf_Macro *macro, Dwarf_Word *paramp)
     return -1;

   Dwarf_Attribute param;
-  if (dwarf_macro_param (macro, 1, &param) != 0)
+  if (dwarf_macro_param (macro, 0, &param) != 0)
     return -1;

   return dwarf_formudata (&param, paramp);

Also, I added this:

@@ -896,13 +896,13 @@ extern int dwarf_macro_getparamcnt (Dwarf_Macro *macro, 
size_t *paramcntp);
 extern int dwarf_macro_param (Dwarf_Macro *macro, size_t idx,
                              Dwarf_Attribute *attribute);
 
-/* Return first macro parameter.  This will return -1 if the parameter
-   is not an integral value.  Use dwarf_macro_param for more general
-   access.  */
+/* Return macro parameter with index 0.  This will return -1 if the
+   parameter is not an integral value.  Use dwarf_macro_param for more
+   general access.  */
 extern int dwarf_macro_param1 (Dwarf_Macro *macro, Dwarf_Word *paramp)
      __nonnull_attribute__ (2);
 
-/* Return second macro parameter.  This will return -1 if the
+/* Return macro parameter with index 1.  This will return -1 if the
    parameter is not an integral or string value.  Use
    dwarf_macro_param for more general access.  */
 extern int dwarf_macro_param2 (Dwarf_Macro *macro, Dwarf_Word *paramp,

>> 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.
>
> This is also where I would suggest we use "1" to indicate we are
> starting a "new style" iteration to make sure the user always gets 0xff
> opcodes. If necessary/possible.

Do you want the change in right away?  I'd wait for the committee
decision before making it.  Otherwise we should add dwarf_macro_version
as well.

Thanks,
Petr