Re: [PATCH 7/9] [libbacktrace] Handle DW_FORM_GNU_ref_alt

[Ian Lance Taylor <iant@golang.org>]

On Wed, Jan 16, 2019 at 4:17 PM Tom de Vries <tdevries@suse.de> wrote:
>
> this handles DW_FORM_GNU_ref_alt which references the .debug_info
> section in the .gnu_debugaltlink file.
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> On 11-12-18 11:14, Tom de Vries wrote:
> > 2018-12-10  Tom de Vries  <tdevries@suse.de>
> >
> >       * dwarf.c (enum attr_val_encoding): Add ATTR_VAL_REF_ALT_INFO.
> >       (struct unit): Add low and high fields.
> >       (struct unit_vector): New type.
> >       (struct dwarf_data): Add units and units_counts fields.
> >       (read_attribute): Handle DW_FORM_GNU_ref_alt using
> >       ATTR_VAL_REF_ALT_INFO.
> >       (find_unit): New function.
> >       (find_address_ranges): Add and handle unit_tag parameter.
> >       (build_address_map): Add and handle units_vec parameter.
> >       (read_referenced_name_1): Handle DW_FORM_GNU_ref_alt.
> >       (build_dwarf_data): Pass units_vec to build_address_map.  Store resulting
> >       units vector.


> > @@ -281,6 +283,10 @@ struct unit
> >    /* The offset of UNIT_DATA from the start of the information for
> >       this compilation unit.  */
> >    size_t unit_data_offset;
> > +  /* Start of the compilation unit.  */
> > +  size_t low;
> > +  /* End of the compilation unit.  */
> > +  size_t high;

The comments should say what low and high are measured in, which I
guess is file offset.  Or is it offset from the start of UNIT_DATA?
Either way, If that is right, then the fields should be named
low_offset and high_offset.  Otherwise it seems easy to confuse with
function_addrs, where low and high refer to PC values.

Also if they are offsets from UNIT_DATA then size_t is OK, but if the
are file offsets they should be off_t.


> > @@ -2144,6 +2198,22 @@ read_referenced_name_1 (struct dwarf_data *ddata, struct unit *u,
> >        || val->encoding == ATTR_VAL_REF_UNIT)
> >      return read_referenced_name (ddata, u, val->u.uint, error_callback, data);
> >
> > +  if (val->encoding == ATTR_VAL_REF_ALT_INFO)
> > +    {
> > +      struct unit *alt_unit
> > +     = find_unit (ddata->altlink->units, ddata->altlink->units_count,
> > +                  val->u.uint);
> > +      if (alt_unit == NULL)
> > +     {
> > +       error_callback (data,
> > +                       "Could not find unit for DW_FORM_GNU_ref_alt", 0);

s/Could/could/

or maybe just skip this error_callback call as discussed earlier.


> > +       return NULL;
> > +     }
> > +      uint64_t unit_offset = val->u.uint - alt_unit->low;

Earlier a unit_offset was the offset of the unit within unit_data, but
here this is an offset within a single unit.  This should just be
called offset, which is the name used by read_referenced_name.

This is OK with those changes.

Thanks.

Ian