Re: [PATCH][v3] Support .debug_macro

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2014-11-04 at 17:30 +0100, Petr Machata wrote:
> 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.

Yes, thanks.

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

I wish we heard something back from the DWARF committee. That would make
decisions a little easier.

If it is reserved then we are indeed done (and we could remove the IMHO
somewhat alarming text from the documentation).

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.

Does that make sense?

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

Those three failures are really a valgrind bug. Valgrind gets confused
when we open and close a shared library file (glibc in all cases I
believe) read-only for inspection, that is also opened already because
the program itself linked against it.
https://bugs.kde.org/show_bug.cgi?id=327427
(That bug report has a lame workaround to get the tests to pass.)

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

Perfect.

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

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

Yes, please.

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

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.

All your changes look good.

Thanks,

Mark