RE: [Patch] Gas support for MIPS Compact EH

["Moore, Catherine" <Catherine_Moore at mentor dot com>]

Hi Richard,

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> 
> > "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > @@ -1433,6 +1441,7 @@ struct bfd_elf_section_data
> 
> > +/* Add a .eh_frame_entry section.  */
> > +
> > +static void
> > +bfd_elf_remember_eh_frame_entry (struct eh_frame_hdr_info
> *hdr_info,
> > +				 asection *sec)
> > +{
> > +  if (hdr_info->array_count == hdr_info->allocated_entries)
> > +    {
> > +      if (hdr_info->allocated_entries == 0)
> > +	{
> > +	  hdr_info->allocated_entries = 2;
> > +	  hdr_info->entries = bfd_malloc (hdr_info->allocated_entries
> > +					  * sizeof(hdr_info->entries[0]));
> > +	}
> > +      else
> > +	{
> > +	  hdr_info->allocated_entries *= 2;
> > +	  hdr_info->entries = bfd_realloc (hdr_info->entries,
> > +	    hdr_info->allocated_entries * sizeof(hdr_info->entries[0]));
> 
> Space before "sizeof" (both times).
> 
> > +	}
> > +
> > +      BFD_ASSERT (hdr_info->entries);
> > +    }
> > +
> > +  hdr_info->entries[hdr_info->array_count++] = sec; }
> > +
> > +/* Parse a .eh_frame_entry section.  Figure out which text section it
> > +   references.  */
> > +
> > +void
> > +_bfd_elf_parse_eh_frame_entry (bfd *abfd, struct bfd_link_info *info,
> > +			       asection *sec, struct elf_reloc_cookie *cookie,
> > +			       bfd_boolean remember)
> 
> This does more than the comment says and the name implies; the
> REMEMBER stuff isn't mentioned.
> 
> The patch tries to do the parsing during bfd_elf_discard_info, but since the
> parsing wants to be able to fail with an error, I think we need to do it in an
> earlier pass.  We can then return a bfd_boolean success code and propagate
> error returns up, which the current patch doesn't do.
> Ideally we'd put the pass somewhere before GC, so that both the GC and
> bfd_elf_discard_info stages can assume parsed .eh_frame_entry sections.
> 
It looks like the legacy dwarf code parse .eh_frame sections during both GC and bfd_elf_discard_info.
There is some shared code between the legacy and compact parsing and it feels awkward to keep legacy eh frame parsing as is and move compact eh frame parsing to an earlier pass.
I could post a patch that moves legacy parsing to an earlier pass to address this.  Do you think that's reasonable?
The other option is to remove the compact eh_frame_entry parsing only (as you suggested).  WDYT?
Thanks,
Catherine

> Having bfd_elf_discard_info add info (as per REMEMBER == TRUE) seems a
> bit counterintuitive.  I think the earlier pass should record all
> .eh_frame_entry sections and then the code currently in
> _bfd_elf_end_eh_frame_parsing (but see below) should remove unwanted
> entries from the eh_frame_hdr_info array.
> 
> > +  if (r_symndx >= cookie->locsymcount
> > +      || ELF_ST_BIND (cookie->locsyms[r_symndx].st_info) != STB_LOCAL)
> > +    {
> > +      h = cookie->sym_hashes[r_symndx - cookie->extsymoff];
> > +      while (h->root.type == bfd_link_hash_indirect
> > +             || h->root.type == bfd_link_hash_warning)
> > +        h = (struct elf_link_hash_entry *) h->root.u.i.link;
> > +
> > +      if (h->root.type != bfd_link_hash_defined
> > +          && h->root.type != bfd_link_hash_defweak)
> > +	goto fail;
> > +
> > +      text_sec = h->root.u.def.section;
> > +    }
> > +  else
> > +    {
> > +      Elf_Internal_Sym *isym;
> > +
> > +      /* Need to: get the symbol; get the section.  */
> > +      isym = &cookie->locsyms[r_symndx];
> > +      text_sec = bfd_section_from_elf_index (abfd, isym->st_shndx);
> > +    }
> 
> Looks like this was lifted from elflink.c.  Please separate it out into a
> subroutine that both sites can use.  E.g.:
> 
> asection *
> _bfd_elf_section_for_symbol (struct elf_reloc_cookie *cookie,
> 			     unsigned long r_symndx)
> {
> ...
> }
> 
> returning null if not known.
> 
> > +fail:
> > +  (*_bfd_error_handler) (_("%B: failed to precoess .eh_frame_entry"),
> > +sec->owner);
>