This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [Patch] Gas support for MIPS Compact EH
- From: "Moore, Catherine" <Catherine_Moore at mentor dot com>
- To: Richard Sandiford <rdsandiford at googlemail dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Thu, 6 Mar 2014 17:44:14 +0000
- Subject: RE: [Patch] Gas support for MIPS Compact EH
- Authentication-results: sourceware.org; auth=none
- References: <FD3DCEAC5B03E9408544A1E416F11242F8FC5972 at NA-MBX-01 dot mgc dot mentorg dot com> <87k3me9jia dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242012EAC2AE0 at NA-MBX-01 dot mgc dot mentorg dot com> <8738jt5zt1 dot fsf at talisman dot default>
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);
>