Re: [PATCH][v2] Support .debug_macro

[Mark Wielaard <mjw at redhat dot com>]

On Mon, 2014-09-29 at 17:26 +0200, Petr Machata wrote:
> Mark Wielaard <mjw@redhat.com> writes:
> 
> > On Fri, 2014-09-12 at 20:03 +0200, Petr Machata wrote:
> >> - dwarf_getmacros is left to support only .debug_macinfo.  The only
> >>   problematic opcode really is DW_MACINFO_vendor_ext
> >
> > You could suggest that the standard reserves that value to enable
> > consumers to use the same parsers for both.
> 
> That's worth a try.

Thanks.

> > Could you also include a small testcase? That could then also be used as
> > an example. I don't think it is immediately clear how you would use the
> > dual style die/off accessors. So an test/example would be helpful.
> 
> I posted a test case patch with my previous submission.  That's still
> active, but I didn't ping it until this one gets through:
> 
>         https://lists.fedorahosted.org/pipermail/elfutils-devel/2014-September/004143.html

Sorry, I missed that one. It does look like what I was hoping for.

> > And I was wondering if having a readelf --debug-dump=decodedmacro using
> > this interface was useful. The raw macro dump doesn't handle transparent
> > includes (you don't have to implement that, but maybe it is useful to do
> > if you do decide to include a test/example anyway).
> 
> Sure, why not.  I actually think readelf should just use libdw/libelf
> interfaces instead of duplicating the decoding logic, though that's not
> what you are proposing.

I admit that the "raw" and "decoded" variant of the macro sections are
mostly the same in this case. I think in this case only the include
magic would be different. The raw variant would just show the macro
names and params as is, while the decoded variant does the actual
transparent include magic and produces the "full list". But in general
it is nice that eu-readelf has a "raw" data parsing and dump of the
section data that you can then compare with what the "decoded" variant
based on libdw functions produces. An extreme example is the
--debug-dump=line vs --debug-dump=decodedline.

> > Why you decide to a include the version and line_offset interfaces on
> > Dwarf_Macro? Isn't that a CU DIE concept? (See also below, maybe I just
> > misunderstand the data representation.)
> 
> Both version and line offset belong to macro units (not CU's, each macro
> unit may set its own .debug_line offset, and macro units have their own
> version).  But there's a question of how to get to that value.  You need
> version to interpret the opcode, and you need .debug_line offset to
> interpret file names.

Ah, right, it is Macro Unit, not Compile Unit. Sorry for confusing them.
That makes my comments even more cryptic :) The question is does the
user of Dwarf_Macro need them? The user isn't going to check against the
version, they are just matching against the DW_MACRO constants and for
interpreting the arguments and file names they will use the attribute
and helper functions provided.

For the version I guess this depends on whether we can treat
DW_MACINFO_vendor_ext specially or not.

So if possible I wouldn't expose them at all in the interface and just
treat them as implementation details. Does that make sense?

> >> +static inline Dwarf_Macro_Op_Table *
> >> +get_macinfo_table (void)
> >> +{
> >> +  static Dwarf_Macro_Op_Table *ret = NULL;
> >> +  if (unlikely (ret == NULL))
> >> +    ret = init_macinfo_table ();
> >> +  return ret;
> >> +}
> >
> > The statics here make me a little uncomfortable. What if the user is
> > working with multiple Dwarfs at the same time iterating through the
> > simultaneously (maybe to compare the macros each defines in their CUs).
> > Won't they overwrite each others Macro_Op_Table?
> 
> This is a singleton macro prototype table for .debug_macinfo (not
> .debug_macro) section.  It's the same in all Dwarf's.  The reason it's
> there is just for uniformness of decoding both types of sections.  So
> there's no problem if you have more than one Dwarf.

Aha, thanks. I misunderstood. Yes, for .debug_macinfo they cannot be
redefined so no issue there.

> One problem this could cause however is that in a multi-threaded
> application, there's a race between the threads for initialization of
> this global resource.  If this is deemed a problem, we could do this:
> 
> __thread Dwarf_Macro_Op_Table *ret = NULL;
> if (unlikely (ret == NULL))
>   {
>     lock;
>     static Dwarf_Macro_Op_Table *global_table = NULL;
>     if (global_table == NULL)
>       global_table = init_macinfo_table ();
>     ret = global_table;
>     unlock;
>   }
> 
> This is essentially double-checked locking, but that's not thread-safe,
> so I make ret thread-local, and always lock before touching the global
> resource.  That should be both thread-safe, have reasonable performance,
> and not waste memory by storing one table for each thread.
> 
> Initially I dismissed this problem because we all know that libdw is
> thread unsafe anyway.  But here we potentially get thread-unsafe
> behavior even if we touch different Dwarf's, which might be unexpected.
> May be worth reconsidering.

Can the table be setup by _init() by marking init_macinfo_table with
__attribute__ ((constructor))? It seems small enough to not really
matter that it is done eagerly instead of lazily when macros are used
and solves all the locking trickery.

> >> -      switch (opcode)
> >> +      /* A fake CU with bare minimum data to fool dwarf_formX into
> >> +	 doing the right thing with the attributes that we put
> >> +	 out.  */
> >> +      Dwarf_CU fake_cu = {
> >> +	.dbg = dbg,
> >> +	.version = 4,
> >> +	.offset_size = table->is_64bit ? 8 : 4,
> >> +      };
> >
> > Do we want/need to match the version here to match the CU DIE this macro
> > table came from? Might be hard to get at at this point though. Maybe
> > match it to the Macro_Op_Table version?
> 
> The problem is that some macro units don't have a related CU per se.
> They could be accessed through offset from other macro units.  There
> typically will be more than one CU that these independent units
> logically belong to, otherwise this transparent inclusion scheme
> wouldn't be advantageous.

And again I confused CU and MU.

> But I guess we should match the CU version to whatever corresponds to
> the .debug_macro version that this macro comes from (the two versions
> can in theory evolve independently.  It's hard to imagine something like
> that happening, but there could be a version of Dwarf that doesn't
> change .debug_info at all, but does change .debug_macro).

I am fine with either hard coding 4 for now or pick the MU version.
Please just add a comment for future reference.

> >> +      Dwarf_Macro macro = {
> >> +	.line_offset = table->line_offset,
> >> +	.version = table->version,
> >> +	.opcode = opcode,
> >> +	.nargs = proto->nforms,
> >> +	.attributes = attributes,
> >> +      };
> >
> > Couldn't you just store the table, opcode and attributes in Dwarf_Macro?
> > The rest can then be retrieved from the associated table.
> 
> Isn't that what I'm doing?  I guess I don't understand what you mean.

This is just an implementation detail, so it doesn't really matter that
much. I just had a bit of a hard time keeping track of which value came
from where and thought it was easier to understand (and less value
copying) if the struct looked like:

 struct Dwarf_Macro_s
 {
   Dwarf_Macro_Op_Table *table;
   uint8_t opcode;
   Dwarf_Attribute *attributes;
 };

And then explicitly lookup line_offset, version and nargs using the
table when using a Dwarf_Macro_s later in the code. Certainly not a big
deal, I don't think the data representation really matters here. Just
wondering why you had chosen this one.

Thanks,

Mark