This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH][v3] Support .debug_macro
- From: Petr Machata <pmachata at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Fri, 07 Nov 2014 20:19:26 +0100
- Subject: Re: [PATCH][v3] Support .debug_macro
Mark Wielaard <mjw@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, ¶m) != 0)
>> >> + return -1;
>> >
>> > Shouldn't this call dwarf_macro_param (macro, 2, ¶m) ?
>>
>> 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, ¶m) != 0)
+ if (dwarf_macro_param (macro, 0, ¶m) != 0)
return -1;
return dwarf_formudata (¶m, 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