Re: [PATCH] Dwarf 5: Handle debug_str_offsets and indexed attributes that have base offsets.

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Ali,

Just a few more comments.  We should be close nowthanks a lot for enduring my nitpicks :).
> @@ -7182,7 +7200,35 @@ lookup_signatured_type (struct dwarf2_cu *cu, ULONGEST sig)
>        return entry;
>      }
>  }
> -
> +
> +/* Return the address base of the compile unit, which, if exists, is stored
> +   either at the attribute DW_AT_GNU_addr_base, or DW_AT_addr_base.  */
> +static gdb::optional<ULONGEST>
> +lookup_addr_base (struct die_info *comp_unit_die)
> +{
> +  struct attribute *attr;
> +  attr = dwarf2_attr_no_follow (comp_unit_die, DW_AT_addr_base);
> +  if (attr == nullptr)
> +    attr = dwarf2_attr_no_follow (comp_unit_die, DW_AT_GNU_addr_base);
> +  if (attr == nullptr)
> +    return gdb::optional<ULONGEST> ();
> +  return DW_UNSND (attr);
> +}
> +
> +/* Return range lists base of the compile unit, which, if exists, is stored
> +   either at the attribute DW_AT_rnglists_base or DW_AT_GNU_ranges_base.  */
> +ULONGEST
> +lookup_ranges_base (struct dwarf2_cu *cu, struct die_info *comp_unit_die)

Remove the `cu` parameter.

> @@ -18481,10 +18530,26 @@ read_full_die_1 (const struct die_reader_specs *reader,
>       attributes.  */
>    die->num_attrs = abbrev->num_attrs;
>  
> +  std::vector<int> indexes_that_need_reprocess;
>    for (i = 0; i < abbrev->num_attrs; ++i)
> -    info_ptr = read_attribute (reader, &die->attrs[i], &abbrev->attrs[i],
> -			       info_ptr);
> +    {
> +      bool need_reprocess;
> +      info_ptr =
> +        read_attribute (reader, &die->attrs[i], &abbrev->attrs[i],
> +			info_ptr, &need_reprocess);
> +      if (need_reprocess)
> +        indexes_that_need_reprocess.push_back (i);
> +    }
> +
> +  struct attribute *attr = dwarf2_attr_no_follow (die, DW_AT_str_offsets_base);
> +  if (attr != nullptr)
> +    cu->str_offsets_base = DW_UNSND (attr);
>  
> +  auto maybe_addr_base = lookup_addr_base(die);
> +  if (maybe_addr_base.has_value ())
> +    cu->addr_base = *maybe_addr_base;

Could this be

  cu->addr_base = lookup_addr_base (die);

?

> @@ -18996,12 +19061,22 @@ partial_die_info::read (const struct die_reader_specs *reader,
>    int has_high_pc_attr = 0;
>    int high_pc_relative = 0;
>  
> +  std::vector<struct attribute> attr_vec (abbrev.num_attrs);
> +  std::vector<int> indexes_that_need_reprocess;
>    for (i = 0; i < abbrev.num_attrs; ++i)
>      {
> -      struct attribute attr;
> -
> -      info_ptr = read_attribute (reader, &attr, &abbrev.attrs[i], info_ptr);
> +      bool need_reprocess;
> +      info_ptr = read_attribute (reader, &attr_vec[i], &abbrev.attrs[i],
> +				 info_ptr, &need_reprocess);
> +      /* String and address offsets that need to do the reprocessing have
> +         already been read at this point, so there is no need to wait until
> +	 the loop terminates to do the reprocessing.  */
> +      if (need_reprocess)
> +        read_attribute_reprocess (reader, &attr_vec[i]);
> +    }
>  
> +  for (attribute &attr : attr_vec)
> +    {
>        /* Store the data if it is of an attribute we want to keep in a
>           partial symbol table.  */
>        switch (attr.name)

I might just have a brain fart at the moment but... what is the reason we
need to have two separate loops here again?  What's the reason you can't just
add the read_attribute_reprocess call in the existing loop?

> @@ -19443,12 +19518,52 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
>    fixup_called = 1;
>  }
>  
> +/* Process the attributes that had to be skipped in the first round. These
> +   attributes are the ones that need str_offsets_base or addr_base attributes.
> +   They could not have been processed in the first round, because at the time
> +   the values of str_offsets_base or addr_base may not have been known.  */
> +void read_attribute_reprocess (const struct die_reader_specs *reader,
> +			       struct attribute *attr)
> +{
> +  struct dwarf2_cu *cu = reader->cu;
> +  switch (attr->form)
> +    {
> +      case DW_FORM_addrx:
> +      case DW_FORM_GNU_addr_index:
> +        DW_ADDR (attr) = read_addr_index (cu, DW_UNSND (attr));
> +        break;
> +      case DW_FORM_strx:
> +      case DW_FORM_strx1:
> +      case DW_FORM_strx2:
> +      case DW_FORM_strx3:
> +      case DW_FORM_strx4:
> +      case DW_FORM_GNU_str_index:
> +	{
> +	  unsigned int str_index = DW_UNSND (attr);
> +	  if (reader->dwo_file != NULL)
> +	    {
> +	      DW_STRING (attr) = read_dwo_str_index (reader, str_index);
> +	      DW_STRING_IS_CANONICAL (attr) = 0;
> +	    }
> +	  else if (cu->str_offsets_base.has_value ())
> +	    {
> +	      DW_STRING (attr) = read_stub_str_index (cu, str_index);
> +	      DW_STRING_IS_CANONICAL (attr) = 0;
> +	    }

What happens if we encounter one these indirect string forms (strx and company) here, but
we don't enter any of the two "if" clauses above?  If that happens, it means we'll leave
a string-type attribute with its `DW_STRING` unset, and it will just break later.

Is it:

- a GDB bug, in which case we should add a gdb_assert_not_reached?
- Wrong DWARF, in which case we should add a complain?
- something else?

Simon