Re: [Patch] Gas support for MIPS Compact EH

[Richard Sandiford <rdsandiford@googlemail.com>]

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> 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.

Right.  But that parsing is allowed to fail, in which case we just
concatenate the input .eh_frames together as normal.  The parsing for
the compact scheme can't fail since the linker needs to understand
the incoming .eh_frame_entry sections in order to sort them correctly.

> 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 think it's OK.  The schemes really are significantly different
in terms of what the linker has to do.

> 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?

Yeah, I think we should leave the current EH scheme as it is now and
only parse .eh_frame_entry sections in advance, so that the data structures
are already available at GC and bfd_elf_discard_info time.

Thanks,
Richard