Re: [RFA/DWARF] constant class of DW_AT_high_pc is offset for version >=4 only.

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2014-02-18 at 19:49 +0100, Joel Brobecker wrote:
> First of all, thanks for the comments!

Just trying to make up for breaking your setup with my original patch.
Although I might be too pedantic in my DWARF spec reading and I cannot
actually approve the patch. So it might be of little help. Sorry about
that.

> On Tue, Feb 18, 2014 at 05:02:49PM +0100, Mark Wielaard wrote:
> > On Tue, 2014-02-18 at 14:30 +0100, Joel Brobecker wrote:
> > >   1. The introduction of attr_value_as_address, to be used
> > >      in place of DW_ADDR when dealing with address attributes.
> > >      I left a few uses of this macro in the situations where
> > >      we actually know that the form is an address form.
> > 
> > This accepts any form as address/unsigned. I would at least check that
> > it is either DW_FORM_data4 or DW_FORM_data8 (even better would be to
> > check the CU address width too, although that would require to pass
> > around cu too, which might not be practical).
> 
> I don't mind adding that, but the real question is what would we do
> if we found an unexpected size? We could emit a complaint, but
> I wouldn't necessarily stop processing the unit's debugging info.
> I think the guideline here is that we should try to do our best
> within reasonable constraints. I think adding a complaint, here,
> is of marginal value as presumably the address read in the odd
> format might be correct. On the other hand, any dump of the debugging
> info should quickly reveal the format used for those addresses.

I think emitting a complaint is the right thing to do. IMHO this really
is broken DWARF and accepting random DW_FORM_foo here might hide real
issues (which as you showed might accidentally seem to work just because
the union values overlap and produce something that is slightly but not
completely wrong).

> > Also I would add a comment
> > that this is really to work around buggy producers.
> 
> I will definitely add a comment saying something about some compiler
> producing odd debugging info. It seems logical to me that address
> attributes would naturally be encoded using an address form, but is
> a constant form clearly forbidden by the (older) standard(s)?
> I would therefore just label the format chosen as unusual. :-).

Sadly DWARF doesn't seem to forbid anything. If it doesn't seem to make
sense then a producer and consumer just have to come to an agreement
about the meaning somehow. But even DWARF2 says that the only possible
encoding of attribute values of class address is DW_FORM_addr.

> > Note that this might break for DWARF5. See http://dwarfstd.org/ShowIssue.php?issue=120719.1
> 
> Interesting. I am curious why you would handle this attribute as
> an offset even when the value is encoded in address form?

As Doug said, it is a space saving (offsets are often small) and it
saves a relocation (linkers have to resolve all DW_FORM_addr values and
they add up).

A more general form of this saving is the DW_FORM_GNU_addr_index
extension which is also proposed as a DWARF5 update:
http://dwarfstd.org/ShowIssue.php?issue=130313.2

> But regardless, I think my change doesn't break the current behavior;
> and to support the DWARF5 standard, the behavior implemented by
> the function is still correct (reading the data from the correct
> union field). You will need to add some code right after the call,
> regardless, which adds the base address if version >= 5.

Yes, agreed.

> > In general I would only use attr_value_as_address for attributes (low_pc
> > and high_pc) which you know a buggy producer might encode with
> > DW_FORM_data[48].
> 
> I think this suggestion goes against the spirit of trying to do our
> best. Not using the function means reading the attribute value from
> the wrong field of the attribute union, which means increasing our
> chances of getting it wrong. On the other hand, using the function
> might allow us to read the correct address and get things to actually
> work.

Be liberal in what you accept. Yeah. OK.

I admit I am mostly worried because GDB is seen as the gold standard of
DWARF consumers. When GDB accepts some DWARF then basically all other
DWARF consumers have to adapt. And in this case I had some trouble
recently with producers and consumers not agreeing on the meaning of
DW_FORM_data[1248] (which I still have to report to the DWARF
committee), so I am a little hyper-sensitive to accepting even more
stuff encoded as DW_FORM_data... sorry about that.

I do think your patch is basically fine. I am just pedantic about
interpreting the DWARF standard. Because I do worry this will make
things harder in the future.

> Also, I don't have acces to said compiler, so I can't try to study
> its output and list the attributes using this constant form. But
> if the low/hi pc attributes use a constant form, why not the
> entry_pc?

OK, that makes things even harder. But could you ask whether they encode
addresses always as DW_FORM_data[48] and whether an update will produce
DW_FORM_addr? It would be good to know if this is just historical and
will not be an issue in the future.

Thanks,

Mark