[Patch] Gas support for MIPS Compact EH

Moore, Catherine Catherine_Moore@mentor.com
Mon Nov 17 16:10:00 GMT 2014


Hi Richard,
Please find the updated Compact EH  patch for binutils. 
This will likely still be a hard patch to review, but I tried to address the many comments that you made earlier.
The main difference is the exception handling relocation type for the Linux toolchain.  It is no longer gprel based. 
The ELF and Linux tools both use pcrel32 and the EH-specific relocation type has been removed. 
I ran into a few issues with the recent changes to eh_frame handling for DWARF, I hopefully covered those with the Compact EH implementation. 
Please let me know what you think.  I hope we are close to converging on an implementation.
Thanks,
Catherine


> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Saturday, February 08, 2014 11:34 AM
> To: Moore, Catherine
> Cc: binutils@sourceware.org
> Subject: Re: [Patch] Gas support for MIPS Compact EH
> 
> Thanks for the updates.
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> >> > @@ -4514,6 +4517,23 @@ argument is not present, otherwise secon  or
> >> > a symbol name.  The default after @code{.cfi_startproc} is
> >> > @code{.cfi_lsda 0xff},  no LSDA.
> >> >
> >> > +@section @code{.cfi_inline_lsda} [@var{align}]
> >> > +@code{.cfi_inline_lsda} marks the start of a LSDA data section and
> >> > +switches to the corresponding @code{.gnu.extab} section.
> >> > +It must be preceded by a CFI block containing a @code{.cfi_lsda}
> >> > +directive and is only valid when generating compact EH frames (i.e.
> >> > +with @code{.cfi_sections eh_frame_entry}.
> >> > +
> >> > +If a compact encoding is being used then the table header and
> >> > +unwinding opcodes will be generated at this point, so that they
> >> > +are immediately followed by the LSDA data.  The symbol referenced
> >> > +by the @code{.cfi_lsda} directive should still be defined in case
> >> > +a fallback FDE based encoding is used.
> >> > +
> >> > +The optional @var{align} argument specifies the alignment required.
> >> > +The alignment is specified as a power of two, as with the
> >> > +@code{.p2align} directive.
> >>
> >> Hmm, switching sections and emitting data feels very different in
> >> style from the other .cfi directives, which just annotate code
> >> without changing the flow of assembly.  I'd like to know other people's
> thoughts on this.
> >>
> >> Also, how do you terminate the LSDA?  The documentation doesn't say
> >> (but should :-)) and I couldn't see this directive in the spec either.
> >
> > The LSDA is terminated by a section switch.
> 
> OK.  Like I say, please mention this in the documentation.
> 
> >> "If a compact encoding is being used" seems redundant, since it comes
> >> just after the bit saying "only valid when generating compact EH frames".
> >>
> >> There need to be tests for all of this.
> >
> > Tests are now included in the patch.
> >
> >> TBH, without tests, and without an
> >> explanation of what the code is doing, I found this patch pretty hard
> >> to review.  E.g.:
> >>
> >> > @@ -129,7 +140,12 @@ get_debugseg_name (segT seg, const char
> >> >        dot = strchr (name + 1, '.');
> >> >
> >> >        if (!dollar && !dot)
> >> > -	name = "";
> >> > +	{
> >> > +	  if (compact_eh && strcmp (name, ".text") != 0)
> >> > +	    return concat (base_name, ".", name, NULL);
> >> > +
> >> > +	  name = "";
> >> > +	}
> >>
> >> why is this change needed?  I.e., for a text section called something
> >> like .foobar, why does compact_eh need to put things in a section
> >> name ending in "..foobar", rather than in the main EH section?  I
> >> assume the double dots are deliberate, e.g. to avoid confusion with
> ".text.foobar"?
> >
> > I'm not sure why Paul put this is and the next hunk.  There were test
> > failures without.  I can revisit this For the next iteration if
> > necessary.
> 
> Yeah, if you could that'd be great.  The code can't really go in if there's no-
> one around who understands what it does.
> 
> I assume it's just to ensure that each text section has its own
> .eh_frame_entry, but in that case I think we should check based on the
> base_name rather than compact_eh.  Or do we need the same treatment
> for .gnu_extab.  If so, why?
> 
> A comment is needed at the very least.
> 
> >> > @@ -161,6 +177,9 @@ alloc_debugseg_item (segT seg, int subse
> >> > static segT  is_now_linkonce_segment (void)  {
> >> > +  if (compact_eh)
> >> > +    return now_seg;
> >> > +
> >> >    if ((bfd_get_section_flags (stdoutput, now_seg)
> >> >         & (SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD
> >> >  	  | SEC_LINK_DUPLICATES_ONE_ONLY |
> >> SEC_LINK_DUPLICATES_SAME_SIZE
> >>
> >> Why is this change needed?
> >>
> >> > @@ -180,7 +199,11 @@ make_debug_seg (segT cseg, char *name, i
> >> >    segT r;
> >> >    flagword flags;
> >> >
> >> > +#ifdef tc_make_debug_seg
> >> > +  r = tc_make_debug_seg (cseg, name); #else
> >> >    r = subseg_new (name, 0);
> >> > +#endif
> >>
> >> Why is this change needed?  And what does the new hook do?  It should
> >> be documented in internals.texi.
> >>
> >> Do we really want to change the behaviour for traditional DWARF EH too?
> >>
> >
> > It looks like this was intended to make .group sections so that text,
> > extab and eh_frame entries are only deleted together.  This is now
> > removed and this new patch add some linker smarts to handle things.
> 
> Thanks.
> 
> >> > @@ -833,14 +859,15 @@ dot_cfi_personality (int ignored ATTRIBU
> >> >      }
> >> >
> >> >    if ((encoding & 0xff) != encoding
> >> > -      || ((encoding & 0x70) != 0
> >> > +      || ((((encoding & 0x70) != 0
> >> >  #if CFI_DIFF_EXPR_OK || defined tc_cfi_emit_pcrel_expr
> >> > -	  && (encoding & 0x70) != DW_EH_PE_pcrel
> >> > +	   && (encoding & 0x70) != DW_EH_PE_pcrel
> >> >  #endif
> >> >  	  )
> >> >  	 /* leb128 can be handled, but does something actually need it?  */
> >> > -      || (encoding & 7) == DW_EH_PE_uleb128
> >> > -      || (encoding & 7) > DW_EH_PE_udata8)
> >> > +	   || (encoding & 7) == DW_EH_PE_uleb128
> >> > +	   || (encoding & 7) > DW_EH_PE_udata8)
> >> > +	&& !tc_cfi_special_encoding (encoding)))
> >> >      {
> >> >        as_bad (_("invalid or unsupported encoding in .cfi_personality"));
> >> >        ignore_rest_of_line ();
> >>
> >> What does a "special" encoding mean?  Again, this hook should be
> >> documented in internals.texi.  And do we really want to change the
> >> set of encodings that are allowed for DWARF, even on MIPS systems
> >> that predate compat EH?
> >>
> > Special means that we have code in the backend to emit a reloc for it.
> > In the revised patch, it goes along with Tc_cfi_reloc_for_encoding.
> > It also looks like internals.texi fails to document many of the
> > tc_macros and it doesn't appear to be built into a .info fiie.
> 
> That's no reason not to document new hooks though.  I know I've used
> internals.texi to read more about a hook in the past.  If you don't want to put
> it there though then please at least put it in a comment instead.
> 
> I found this a really hard and time-consuming patch to review.
> That could just be because I'm stupid, but I think you're underestimating how
> difficult this is to follow for someone coming to it new.
> 
> >> Why is the CFI_EMIT_target needed?
> > Sttrictly speaking it isn't, but having a different value was useful
> > in the error checking.
> 
> Specifically, useful in what way?  Please could you add a comment explaining
> why.
> 
> >>
> >>   data_size += encoding_size (fde->per_encoding);
> >>
> >> would work; if not, please extend encoding_size.  We then have:
> >>
> >> > +  if (fde->per_encoding != DW_EH_PE_omit)
> >> > +    {
> >> > +      *(p++) = 0;
> >> > +      md_number_to_chars (p, 0, 4);
> >> > +      tc_cfi_fix_eh_ref (p, &fde->personality);
> >> > +      p += 4;
> >>
> >> where tc_cfi_fix_eh_ref emits an R_MIPS_EH relocation regardless of
> >> which personality encoding is used.  AIUI, the encoding must be one
> >> of the "special" ones allowed by tc_cfi_special_encoding, so we
> >> should check for that.
> >>
> >> The tc_cfi_fix_eh_ref and tc_cfi_emit_expr hooks don't seem very
> >> consistent; the former relies on the caller to clear the bytes,
> >> whereas the latter is supposed to do it itself.
> >
> > All fixed, now using a hook to return a reloc and eliminated the use
> > of R_MIPS_EH from the assembler.
> 
> Hmm, but how does it work under the new scheme?  It looks like gas now
> always emits the .eh_frame_entry sections using R_MIPS_PC32, is that right?
> But the linker chooses the .eh_frame_hdr encoding based on --pcrel-eh-
> reloc, which also controls how R_MIPS_EH is handled.  So if the:
> 
>   DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect
> 
> encoding is chosen for the .eh_frame_entry sections at link time, what
> converts the input sections to use that encoding instead of the original
> R_MIPS_PC32?  I'd assumed R_MIPS_EH was defined the way it was to avoid
> that kind of thing.
> 
> I wasn't expecting you to drop the R_MIPS_EH stuff altogether.
> I was just confused by the way that the old assembler patch seemed to be
> associating R_MIPS_EH with a specific encoding
> (DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect).  So:
> 
> >> Neither of the hooks really seem to be doing anything target-specific
> >> except for the all-important job of picking a reloc.  I think it
> >> would be cleaner to have a hook that returns the reloc instead.
> >> In the case of tc_cfi_emit_expr, this could be done by making
> >> tc_cfi_special_encoding return the reloc for an encoding, or
> >> BFD_RELOC_NONE for unsupported encodings.
> >>
> >> Also, this is more of a design point, but I find the handling of the
> >> personality encoding and R_MIPS_EH handling a bit confusing.  To
> >> quote:
> >>
> >> ---------------------------------------------------------------------
> >> 11 Relocations
> >>
> >> A new static relocation, R_MIPS_EH, is defined. The semantics of this
> >> relocation depend on whether static or dynamic linking is provided.
> >>
> >> A GNU extension using relocation number 249 shall be used. The
> >> relocation address need not be naturally aligned.
> >>
> >> 11.1 Static code
> >>
> >> For static code generation, the calculation is the same as an
> >> R_MIPS_REL32 relocation.
> >>
> >> At runtime, the following expression provides the relocated value, if 'ptr'
> >> points to the relocation location.
> >>
> >> • (ptrdiff_t)ptr + *(int32_t __attribute__((packed))*)ptr
> >>
> >> [...]
> >>
> >> 11.2 PIC code
> >>
> >> For PIC code generation, a 32- or 64-bit GOT-table entry must be
> >> allocated to refer to the (dynamically resolved) target address. Once
> >> the GOT entry has been allocated, the static calculation is as for an
> >> R_MIPS_GPREL32 relocation (except that the symbol is externally visible).
> >> The GOT- slot has an associated R_MIPS_{32,64} dynamic relocation
> >> emitted – and that will of course be at a naturally aligned location
> >>
> >> At runtime, the following expression provides the relocated value, if 'ptr'
> >> points to the relocation location, and 'gp' is the global pointer
> >> value:
> >>
> >> • *(ptrdiff_t *)((ptrdiff_t)gp + *(int32_t __attribute__((packed))
> >> *)ptr)
> >>
> >> [...]
> >> ---------------------------------------------------------------------
> >>
> >> So as I understand the first sentence, it's actually the linker that
> >> decides whether R_MIPS_EH relocations act as direct PC-relative
> >> references (11.1) or as indirect datarel references (11.2).
> >> It's therefore the linker that decides what DWARF encoding R_MIPS_EH
> >> fields use.  The linker then records that choice in the .eh_frame_hdr.
> >> Is that
> >> right?
> >>
> >> If so, the assembler seems to be associating R_MIPS_EH with specific
> >> DWARF encodings, even though the interpretation of R_MIPS_EH isn't
> >> known at that stage.  I.e. the code reads:
> >>
> >> #define tc_cfi_special_encoding(e) \
> >>   ((e) == (DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect)
> \
> >>    || (e) == (DW_EH_PE_sdata4 | DW_EH_PE_pcrel))
> >>
> >> void
> >> mips_cfi_emit_expr (expressionS *exp, int encoding) {
> >>   char *p;
> >>
> >>   p = frag_more(4);
> >>   md_number_to_chars (p, 0, 4);
> >>   if ((encoding & 0x70) == DW_EH_PE_datarel)
> >>     mips_cfi_fix_eh_ref (p, exp);
> >>   else
> >>     {
> >>       fix_new (frag_now, p - frag_now->fr_literal, 4, exp->X_add_symbol,
> >> 	       exp->X_add_number, TRUE, BFD_RELOC_32_PCREL);
> >>     }
> >> }
> >>
> >> where mips_cfi_fix_eh_ref emits an R_MIPS_EH.  So the code appears to
> >> be using R_MIPS_EH for the DWARF encoding:
> >>
> >>     DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect
> >>
> >> even though there's no guarantee that the final value will be either
> >> datarel or indirect.
> >>
> >> Or, to put it another way, why are we making the choice between
> >> R_MIPS_PC32 and R_MIPS_EH at assembly time in mips_cfi_emit_expr,
> but
> >> not in mips_cfi_fix_eh_ref, even though the patch appears to allow
> >> the same two encodings in both casees?
> 
> I thought R_MIPS_EH would be used for the .eh_frame_entry entries only,
> since in that case the actual encoding of the address isn't known until link
> time.  I thought mips_cfi_emit_expr, which deals with specific assembly-time
> encoding types, ought to use specific relocations for those relocation types
> instead.  I.e. mips_cfi_emit_expr would always use
> R_MIPS_PC32 for:
> 
>   DW_EH_PE_sdata4 | DW_EH_PE_pcrel
> 
> and always use a new relocation type for:
> 
>   DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect
> 
> Note that the spec is wrong to say that R_MIPS_GPREL32 is equivalent to the
> above, since R_MIPS_GPREL32 doesn't include any indirection.
> I think we need to define a new reloc such as R_MIPS_GOT_DISP32.
> 
> But more fundamentally, I don't really understand why we want the GOT-
> based encoding for .eh_frame_hdr by default.  Is that intended for systems
> that allow the text and data segments to be moved relative to each other,
> like VxWorks?  It seems like a big hammer on bare-metal and GNU/Linux,
> where the PC-relative form seems better.  And on VxWorks it seems a bad
> idea to eat up valuable GOT space, since VxWorks doesn't have any form of
> multigot support.
> 
> >> > +  demand_empty_rest_of_line ();
> >> > +  ccseg = CUR_SEG (last_fde);
> >> > +  /* Open .gnu_extab section.  */
> >> > +  cfi_seg = get_cfi_seg (ccseg, ".gnu_extab",
> >> > +			 (SEC_ALLOC | SEC_LOAD | SEC_DATA
> >> > +			  | DWARF2_EH_FRAME_READ_ONLY),
> >> > +			 1);
> >> > +#ifdef md_fix_up_eh_frame
> >> > +  md_fix_up_eh_frame (cfi_seg);
> >> > +#else
> >> > +  (void) cfi_seg;
> >> > +#endif
> >> > +
> >> > +  frag_align (align, 0, 0);
> >> > +  record_alignment (now_seg, align);  if (last_fde->eh_header_type
> >> > + == EH_COMPACT_HAS_LSDA)
> >> > +    output_compact_unwind_data (last_fde, align);
> >>
> >> Please could you explain the EH_COMPACT_LEGACY handling here?
> >
> > Would you please clarify this question?  I don't see the reference to
> > EH_COMPACT_LEGACY.
> 
> That was the problem :-)  Further up there's:
> 
>   if (last_fde->eh_header_type != EH_COMPACT_LEGACY
>       && last_fde->eh_header_type != EH_COMPACT_HAS_LSDA)
>     {
>       as_bad (_(".cfi_inline_lsda seen for frame without .cfi_lsda"));
>       ignore_rest_of_line ();
>       return;
>     }
> 
> So what does this code mean/do in the:
> 
>   last_fde->eh_header_type == EH_COMPACT_LEGACY
> 
> case?
> 
> > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h index 0aab5fa..b7d7df0
> > 100644
> > --- a/bfd/elf-bfd.h
> > +++ b/bfd/elf-bfd.h
> > @@ -381,8 +381,10 @@ struct eh_frame_hdr_info  {
> >    struct htab *cies;
> >    asection *hdr_sec;
> > -  unsigned int fde_count, array_count;
> > +  unsigned int fde_count, array_count, allocated_entries;
> >    struct eh_frame_array_ent *array;
> > +  /* eh_frame_entry fragments.  */
> > +  asection **entries;
> >    /* TRUE if we should try to merge CIEs between input sections.  */
> >    bfd_boolean merge_cies;
> >    /* TRUE if all .eh_frames have been parsd.  */
> 
> I'm not sure there's enough commonality between the DWARF and compact
> versions to share this structure.  Either we should have separate structures
> or use a union to separate out the format-specific bits.
> 
> > @@ -1433,6 +1441,7 @@ struct bfd_elf_section_data
> >  #define elf_next_in_group(sec)	(elf_section_data(sec)-
> >next_in_group)
> >  #define elf_fde_list(sec)	(elf_section_data(sec)->fde_list)
> >  #define elf_sec_group(sec)	(elf_section_data(sec)->sec_group)
> > +#define elf_section_eh_frame_entry(sec)	(elf_section_data(sec)-
> >eh_frame_entry)
> 
> Long line.
> 
> > +/* 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.
> 
> 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);
> 
> Long line.
> 
> > +/* Add space for a CANTUNWIND terminator to SEC is the text sections
> > +   referenced by it and NEXT are not contiguous, or NEXT is NULL.  */
> 
> s/is the text/if the text/.
> 
> > +
> > +static void
> > +add_eh_frame_hdr_terminator (asection *sec,
> > +			     asection *next)
> 
> No need for a line break in the arguments.
> 
> > +{
> > +  bfd_vma end;
> > +  bfd_vma next_start;
> > +  asection *text_sec;
> > +
> > +  if (next)
> > +    {
> > +      /* See if there is a gap (presuably a text section without unwind info)
> > +	 between these two entries.  */
> > +      text_sec = (asection *) elf_section_data (sec)->sec_info;
> > +      end = text_sec->output_section->vma + text_sec->output_offset
> > +	    + text_sec->size;
> > +      text_sec = (asection *) elf_section_data (next)->sec_info;
> > +      next_start = text_sec->output_section->vma + text_sec-
> >output_offset;
> > +      /* Allow for alignment padding.  */
> > +      end = BFD_ALIGN (end, 1 << bfd_get_section_alignment (text_sec-
> >owner, text_sec));
> > +      if (end == next_start)
> > +	return;
> > +    }
> > +
> > +  /* Add space for a CANTUNWIND terminator.  */  if (!sec->rawsize)
> > +    sec->rawsize = sec->size;
> > +
> > +  bfd_set_section_size (sec->owner, sec, sec->size + 8); }
> 
> Is the idea to detect when there's "real" text (as opposed to padding)
> between the two text sections?  If so, I don't think aligning the end of SEC's
> text section according to NEXT's is correct.  If NEXT's text section has a
> particularly high alignment then you could have "end == next_start" even if
> there are other text sections inbetween.
> 
> Maybe in the first cut we should just be conservative and compare the start
> and end directly, even if that leads to unnecessary CANTUNWINDs for
> padding.
> 
> > +/* Finish a pass over all .eh_frame and eh_frame_entry sections.  */
> > +
> > +bfd_boolean
> >  _bfd_elf_end_eh_frame_parsing (struct bfd_link_info *info)  {
> >    struct eh_frame_hdr_info *hdr_info;
> > +  unsigned int i;
> >
> >    hdr_info = &elf_hash_table (info)->eh_info;
> >    hdr_info->parsed_eh_frames = TRUE;
> > +
> > +  if (hdr_info->array_count == 0 || info->eh_frame_hdr < 2)
> > +    return FALSE;
> > +
> > +  qsort (hdr_info->entries, hdr_info->array_count,
> > +	 sizeof (asection *), cmp_eh_frame_hdr);
> > +
> > +  for (i = 0; i < hdr_info->array_count - 1; i++)
> > +    {
> > +      add_eh_frame_hdr_terminator (hdr_info->entries[i],
> > +				   hdr_info->entries[i + 1]);
> > +    }
> > +
> > +  /* Add a CANTUNWIND terminator after the last entry.  */
> > + add_eh_frame_hdr_terminator (hdr_info->entries[i], NULL);  return
> > + TRUE;
> 
> This routine is called from both bfd_elf_gc_sections and
> bfd_elf_discard_info but I think you only want it for bfd_elf_discard_info.
> So perhaps this should be a separate function.
> 
> I wonder whether we could instead insert CANTUNWINDs earlier (say in the
> non-dynamic part of size_dynamic_sections) based on the link order.
> I.e. rather than comparing the start and end addresses of two sections, we
> could just walk the sections in the order that they're going to be linked.
> 
> It sounds like doing it that way would be more direct and more efficient.
> Returning TRUE here forces the linker to map sections twice.
> 
> > @@ -1271,6 +1465,45 @@ _bfd_elf_eh_frame_present (struct
> bfd_link_info *info)
> >    return FALSE;
> >  }
> >
> > +/* Return true if there is at least one .eh_frame_entry section in
> > +   input files.  */
> > +bfd_boolean
> > +_bfd_elf_eh_frame_entry_present (struct bfd_link_info *info) {
> > +  asection *o;
> > +  bfd *abfd;
> > +
> > +  for (abfd = info->input_bfds; abfd != NULL; abfd = abfd->link_next)
> > +    {
> > +      for (o = abfd->sections; o; o = o->next)
> > +	{
> > +	  const char *name = bfd_get_section_name (abfd, o);
> > +
> > +	  if (strcmp (name, ".eh_frame_entry")
> > +	      && !bfd_is_abs_section (o->output_section))
> > +	    {
> > +	      if (strcmp (o->output_section->name, ".eh_frame_hdr"))
> > +		return TRUE;
> > +	      else
> > +		{
> > +		  (*_bfd_error_handler)
> > +			(_("error: an '.eh_frame_entry'"
> > +			   " input section that is not mapped to the"
> > +			   " '.eh_frame_hdr' output section was seen"));
> > +		  (*_bfd_error_handler)
> > +			(_("note: try adding '*(.eh_frame_entry"
> > +			   " .eh_frame_entry.*)' to the '.eh_frame_hdr'"
> > +			   " output section description in the linker"
> > +			   " script"));
> > +		  bfd_set_error (bfd_error_invalid_operation);
> > +		  return FALSE;
> > +		}
> > +	    }
> > +	}
> > +    }
> > +  return FALSE;
> 
> The caller can't tell "FALSE because an error was reported" from "FALSE
> because there was no .eh_frame_entry".  We should separate out the two
> cases and propagate error returns up.
> 
> > @@ -1387,6 +1636,71 @@ _bfd_elf_eh_frame_section_offset (bfd
> *output_bfd ATTRIBUTE_UNUSED,
> >  	  + extra_augmentation_data_bytes (sec_info->entry + mid));  }
> >
> > +/* Write out .eh_frame_entry section.  Add CANTUNWIND terminator if
> needed.
> > +   Also check that the contents look sane.  */
> > +
> > +bfd_boolean
> > +_bfd_elf_write_section_eh_frame_entry (bfd *abfd,
> > +				       asection *sec,
> > +				       bfd_byte *contents)
> 
> Formatting: first two arguments fit on a line.
> 
> > +  text_sec = (asection *) elf_section_data (sec)->sec_info;
> > +  addr = text_sec->output_section->vma + text_sec->output_offset
> > +	 + text_sec->size;
> > +  addr &= ~1;
> > +  addr -= (sec->output_section->vma + sec->output_offset +
> > +sec->rawsize);
> > +  BFD_ASSERT ((addr & 1) == 0);
> 
> It looks like this could trigger for odd-sized input text sections.
> I think it should be an error instead of an assert.
> 
> > +  if (last_addr >= addr + sec->rawsize)
> > +    {
> > +      (*_bfd_error_handler) (_("%B: %s points past end of text section"),
> > +			     sec->owner, sec->name);
> > +      return FALSE;
> > +    }
> > +
> > +  if (sec->size == sec->rawsize)
> > +    return TRUE;
> > +
> > +  BFD_ASSERT (sec->size == sec->rawsize + 8);
> > +  BFD_ASSERT ((addr & 1) == 0);
> > +  bfd_put_32 (abfd, addr, cantunwind);
> > +  bfd_put_32 (abfd, 0x015d5d01, cantunwind + 4);
> > +  return bfd_set_section_contents (abfd, sec->output_section,
> cantunwind,
> > +				   sec->output_offset + sec->rawsize, 8);
> 
> ARM and c6x seem to use 0 rather than 0x5d as the "no unwind" opcode, is
> that right?  If so, I think this should be a hook.
> 
> The decision about whether to insert the CANTUNWIND is made during
> bfd_elf_discard_info, but addresses can change after that “thanks”
> to relaxation.  So this could in principle end up emitting a CANTUNWIND for
> the same address as the following text section.
> 
> > @@ -1746,6 +2060,44 @@ vma_compare (const void *a, const void *b)
> >    return 0;
> >  }
> >
> > +/* Reorder .eh_frame_entry sections to match the associated text
> > +sections.  */
> 
> I think a bit more commentary would help here.  E.g.:
> 
> /* Reorder .eh_frame_entry sections to match the associated text sections.
>    This routine is called during the final linking step, just before writing
>    the contents.  At this stage sections in the eh_frame_hdr_info are already
>    sorted in order of increasing text section address and so we simply need
>    to make the .eh_frame_entrys themselves follow that same order.  Note
> that
>    it is invalid for a linker script to try to force a particular order of
>    .eh_frame_entry sections.  */
> 
> However, even so:
> 
> > +
> > +void
> > +_bfd_elf_fixup_eh_frame_hdr (struct bfd_link_info *info) {
> > +  asection *sec = NULL;
> > +  struct eh_frame_hdr_info *hdr_info;
> > +  unsigned int i;
> > +  bfd_vma offset;
> > +  struct bfd_link_order *p;
> > +
> > +  hdr_info = &elf_hash_table (info)->eh_info;
> > +
> > +  if (hdr_info->hdr_sec == NULL || info->eh_frame_hdr < 2
> > +      || hdr_info->array_count == 0)
> > +    return;
> > +
> > +  /* Change section output offsets to be in text section order.  */
> > + offset = 8;  for (i = 0; i < hdr_info->array_count; i++)
> > +    {
> > +      sec = hdr_info->entries[i];
> > +      sec->output_offset = offset;
> > +      offset += sec->size;
> > +    }
> 
> ...this assumes without checking that all .eh_frame_entry sections have the
> same output section, and that there's nothing in that output section besides
> these sections.  We ought to check for that.
> (_bfd_elf_eh_frame_entry_present does some sanity checking, but AFAICT
> only for one input section.)
> 
> We could either do the checking earlier or do it here, e.g. something like:
> 
>   /* Change section output offsets to be in text section order.  */
>   offset = 8;
>   osec = hdr_info->entries[0]->output_section;
>   for (i = 0; i < hdr_info->array_count; i++)
>     {
>       sec = hdr_info->entries[i];
>       if (sec->output_section != osec)
>         ...error...
>       sec->output_offset = offset;
>       offset += sec->size;
>     }
> 
> and:
> 
> > +  /* Fix the link_order to match.  */
> > +  for (p = sec->output_section->map_head.link_order; p != NULL; p = p-
> >next)
> > +    {
> > +      if (p->type != bfd_indirect_link_order)
> > +	abort();
> > +
> > +      p->offset = p->u.indirect.section->output_offset;
> > +      i--;
> > +    }
> 
> if (i != 0)
>   ...error...
> 
> (with bfd_boolean return of course).
> 
> > @@ -1787,6 +2145,36 @@ _bfd_elf_write_section_eh_frame_hdr (bfd
> *abfd, struct bfd_link_info *info)
> >        bfd_size_type size;
> >        bfd_vma encoded_eh_frame;
> >
> > +      if (info->eh_frame_hdr == 2)
> > +	{
> > +	  const struct elf_backend_data *bed;
> > +	  bfd_vma count;
> > +
> > +	  contents = bfd_malloc (8);
> > +	  if (contents == NULL)
> > +	    return FALSE;
> > +
> > +	  if (sec->size != 8)
> > +	    abort ();
> > +
> > +	  memset (contents, 0, 8);
> > +	  contents[0] = 2;
> > +	  bed = get_elf_backend_data (abfd);
> > +	  if (bed->compact_eh_encoding)
> > +	    contents[1] = (*bed->compact_eh_encoding) (info);
> > +	  else
> > +	    contents[0] = 0xff;
> 
> Can the 0xff case actually happen for MIPS?  If not, I think we should assert
> or report an error instead.  It seems wrong to generate a header but silently
> mark it invalid.
> 
> > +
> > +	  count = (sec->output_section->size -9 ) / 8;
> 
> Why -9?  Deserves a comment at least.  The space should be before the 9.
> 
> > +	  bfd_put_32 (abfd, count, contents + 4);
> > +	  retval = bfd_set_section_contents (abfd, sec->output_section,
> > +					     contents,
> > +					     (file_ptr) sec->output_offset,
> > +					     sec->size);
> > +	  free (contents);
> 
> malloc/free seems a bit excessive for 8 bytes.  We might as well just use a
> stack array instead.
> 
> > @@ -10039,6 +10040,10 @@ elf_link_input_bfd (struct elf_final_link_info
> *flinfo, bfd *input_bfd)
> >  	      return FALSE;
> >  	  }
> >  	  break;
> > +	case SEC_INFO_TYPE_EH_FRAME_ENTRY:
> > +	    if (! _bfd_elf_write_section_eh_frame_entry (output_bfd, o,
> contents))
> > +	      return FALSE;
> > +	  break;
> 
> Excess indentation of the "if".
> 
> > @@ -11807,6 +11814,13 @@ _bfd_elf_gc_mark (struct bfd_link_info *info,
> >  	}
> >      }
> >
> > +  eh_frame = elf_section_eh_frame_entry (sec);  if (eh_frame &&
> > + !eh_frame->gc_mark)
> > +    {
> > +    if (!_bfd_elf_gc_mark (info, eh_frame, gc_mark_hook))
> > +      return FALSE;
> > +    }
> 
> In this context we should be using "ret":
> 
>   eh_frame = elf_section_eh_frame_entry (sec);
>   if (ret && eh_frame && !eh_frame->gc_mark)
>     ret = _bfd_elf_gc_mark (info, eh_frame, gc_mark_hook);
> 
> > @@ -12190,22 +12204,42 @@ bfd_elf_gc_sections (bfd *abfd, struct
> bfd_link_info *info)
> >    bed->gc_keep (info);
> >
> >    /* Try to parse each bfd's .eh_frame section.  Point
> elf_eh_frame_section
> > -     at the .eh_frame section if we can mark the FDEs individually.  */
> > +     at the .eh_frame section if we can mark the FDEs individually.
> > +     Establish links from text sections to their corresponding
> > +     .eh_frame_entry sections.  */
> >    _bfd_elf_begin_eh_frame_parsing (info);
> >    for (sub = info->input_bfds; sub != NULL; sub = sub->link_next)
> >      {
> >        asection *sec;
> >        struct elf_reloc_cookie cookie;
> >
> > -      sec = bfd_get_section_by_name (sub, ".eh_frame");
> > -      while (sec && init_reloc_cookie_for_section (&cookie, info, sec))
> > +      if (!init_reloc_cookie (&cookie, info, sub))
> > +	return FALSE;
> > +
> > +      if (info->eh_frame_hdr < 2)
> >  	{
> > -	  _bfd_elf_parse_eh_frame (sub, info, sec, &cookie);
> > -	  if (elf_section_data (sec)->sec_info
> > -	      && (sec->flags & SEC_LINKER_CREATED) == 0)
> > -	    elf_eh_frame_section (sub) = sec;
> > -	  fini_reloc_cookie_for_section (&cookie, sec);
> > -	  sec = bfd_get_next_section_by_name (sec);
> > +	  sec = bfd_get_section_by_name (sub, ".eh_frame");
> > +	  if (sec && init_reloc_cookie_rels (&cookie, info, sub, sec))
> > +	    {
> > +	      _bfd_elf_parse_eh_frame (sub, info, sec, &cookie);
> > +	      if (elf_section_data (sec)->sec_info
> > +		  && (sec->flags & SEC_LINKER_CREATED) == 0)
> > +		elf_eh_frame_section (sub) = sec;
> > +	      fini_reloc_cookie_for_section (&cookie, sec);
> > +	    }
> > +	}
> > +      else
> > +	{
> > +	  for (sec = sub->sections; sec; sec = sec->next)
> > +	    {
> > +	      if (CONST_STRNEQ (bfd_section_name (sub, sec),
> ".eh_frame_entry")
> > +		  && init_reloc_cookie_rels (&cookie, info, sub, sec))
> > +		{
> > +		  _bfd_elf_parse_eh_frame_entry (sub, info, sec, &cookie,
> > +						 FALSE);
> > +		  fini_reloc_cookie_for_section (&cookie, sec);
> > +		}
> > +	    }
> 
> This changes the info->eh_frame_hdr != 2 case to only handle the first
> .eh_frame section.  ("while" -> "if").
> 
> Also, the init/fini_reloc_cookie* calls don't match up.  I assume the idea is to
> avoid excessive allocation and freeing of the locsyms, but in that case you
> should use fini_reloc_cookie_rels instead of fini_reloc_cookie_for_section
> and call fini_reloc_cookie at the end.
> At the moment I think this leaks memory if there are no EH sections.
> 
> I don't know either way whether splitting the init_reloc_cookie_for_section
> call up is a win or not for .eh_frame.  It will be a win if there are multiple EH
> sections but a loss if there are none, since we then initialise and free the bfd-
> level information unnecessarily.  So I think it might make sense to keep the
> .eh_frame code as it is now and restrict the *_rels to the new code.
> 
> > @@ -12681,7 +12717,7 @@ bfd_elf_discard_info (bfd *output_bfd, struct
> bfd_link_info *info)
> >        bed = get_elf_backend_data (abfd);
> >
> >        eh = NULL;
> > -      if (!info->relocatable)
> > +      if ((info->eh_frame_hdr < 2) && !info->relocatable)
> 
> Formatting: no brackets around "info->eh_frame_hdr < 2".
> 
> > @@ -5113,6 +5117,17 @@ mips_elf_relocation_needs_la25_stub (bfd
> *input_bfd, int r_type,
> >        return FALSE;
> >      }
> >  }
> > +
> > +/* On some targets R_MIPS_EH is interpreted as R_MIPS_PC32.  */
> > +static int mips_elf_real_reloc (struct bfd_link_info *info, int
> > +r_type) {
> > +  struct mips_elf_link_hash_table *htab = mips_elf_hash_table (info);
> > +  if (r_type == R_MIPS_EH)
> > +    return htab->eh_reloc_type;
> > +
> > +  return r_type;
> > +}
> 
> This seems a bit error-prone, since if a mips_elf_real_reloc call ("what am I
> really looking at?") gets missed at some point it will still work in the common
> case of R_MIPS_EH being unchanged.  If we do add the
> R_MIPS_GOT_DISP32 suggested above then that problem would go away,
> since R_MIPS_EH would always be replaced by something else.
> 
> > @@ -1648,6 +1649,7 @@ static const pseudo_typeS mips_pseudo_table[]
> =
> >    {"tprelword", s_tprelword, 0},
> >    {"tpreldword", s_tpreldword, 0},
> >    {"gpvalue", s_gpvalue, 0},
> > +  {"forcegpword", s_force_gpword, 0},
> >    {"gpword", s_gpword, 0},
> >    {"gpdword", s_gpdword, 0},
> >    {"ehword", s_ehword, 0},
> 
> Would prefer s_forcegpword, for consistency.
> 
> > @@ -18249,3 +18256,13 @@ tc_mips_regname_to_dw2regnum (char
> *regname)
> >
> >    return regnum;
> >  }
> > +
> > +#if defined (OBJ_ELF)
> > +bfd_reloc_code_real_type
> > +mips_cfi_reloc_for_encoding (int encoding) {
> > +  if ((encoding & 0x70) == DW_EH_PE_datarel)
> > +    return BFD_RELOC_GPREL32;
> > +  return BFD_RELOC_32_PCREL;
> > +}
> > +#endif
> 
> Given the answer to what "special" means above, I think we should combine
> tc_cfi_special_encoding and tc_cfi_reloc_for_encoding into a single hook
> that returns BFD_RELOC_NONE for an encoding that isn't special.
> 
> As mentioned above, I don't think R_MIPS_GPREL32 is right for the datarel-
> indirect case.
> 
> > diff --git a/gas/config/tc-mips.h b/gas/config/tc-mips.h index
> > c7eaa04..a300062 100644
> > --- a/gas/config/tc-mips.h
> > +++ b/gas/config/tc-mips.h
> > @@ -177,7 +177,9 @@ extern enum dwarf2_format mips_dwarf2_format
> > (asection *);
> >
> >  extern int mips_dwarf2_addr_size (void);  #define
> > DWARF2_ADDR_SIZE(bfd) mips_dwarf2_addr_size () -#define
> > DWARF2_FDE_RELOC_SIZE mips_dwarf2_addr_size ()
> > +#define DWARF2_FDE_RELOC_SIZE (compact_eh ? 4 :
> mips_dwarf2_addr_size
> > +()) #define DWARF2_FDE_RELOC_ENCODING(enc) \
> > +  (enc | (compact_eh ? DW_EH_PE_pcrel : 0))
> 
> What case is this handling?  Please explain this a bit more.
> 
> > @@ -4566,6 +4586,22 @@ argument is not present, otherwise second
> > argument should be a constant  or a symbol name.  The default after
> > @code{.cfi_startproc} is @code{.cfi_lsda 0xff},  no LSDA.
> >
> > +@section @code{.cfi_inline_lsda} [@var{align}]
> > +@code{.cfi_inline_lsda} marks the start of a LSDA data section and
> > +switches to the corresponding @code{.gnu.extab} section.
> > +Must be preceded by a CFI block containing a @code{.cfi_lsda} directive.
> > +Only valid when generating compact EH frames (i.e.
> > +with @code{.cfi_sections eh_frame_entry}.
> 
> Missing ")".
> 
> > @@ -4643,6 +4679,20 @@ mark a code segment that has only one return
> > address which is reached  by a direct branch and no copy of the return
> > address exists in memory  or another register.
> >
> > +@section @code{.cfi_epilogue_begin}
> > +A pseudo-operation which marks the beginning of the epilogue in a
> > +given function.  This is currently used by the compact unwind-table
> > +implementation (for exception handling), which assumes a single
> > +register state for unwinding throughout the body of a function.  The
> > +function is scanned, and CFI directives are rewritten in a compact
> > +form.  However, recent versions of GCC emit unwind information for
> > +function epilogues, which should not be represented in this compact
> > +form: hence, any CFI directives which occur in a function after
> > +@code{.cfi_epilogue_end} are not processed.
> > +
> > +This operation only affects the internals of the assembler, and is
> > +not represented in the binary output.
> 
> Don't really follow this, sorry.  Can you give an example, or better yet, a
> testcase?
> 
> > @@ -161,6 +278,9 @@ alloc_debugseg_item (segT seg, int subseg, char
> > *name)  static segT  is_now_linkonce_segment (void)  {
> > +  if (compact_eh)
> > +    return now_seg;
> > +
> 
> Please add a comment explaining why this is correct.
> 
> > -/* Emit a single byte into the current segment.  */
> > -
> > -static inline void
> > -out_one (int byte)
> > +static segT
> > +get_cfi_seg (segT cseg, const char *base, flagword flags, int align)
> >  {
> > -  FRAG_APPEND_1_CHAR (byte);
> > +  if (SUPPORT_FRAME_LINKONCE || ((flags & SEC_DEBUGGING) == 0 &&
> compact_eh))
> > +    {
> 
> Please add a comment here too to explain the SEC_DEBUGGING test.
> 
> > +/* Set the personality id in the FDE structure.  */
> > +
> > +static void
> > +dot_cfi_personality_id (int ignored ATTRIBUTE_UNUSED)
> >  {
> > -  md_number_to_chars (frag_more (2), data, 2);
> > +  struct fde_entry *fde;
> > +
> > +  if (frchain_now->frch_cfi_data == NULL)
> > +    {
> > +      as_bad (_("CFI instruction used without previous .cfi_startproc"));
> > +      ignore_rest_of_line ();
> > +      return;
> > +    }
> > +
> > +  fde = frchain_now->frch_cfi_data->cur_fde_data;
> > +  fde->personality_id = cfi_parse_const ();
> > + demand_empty_rest_of_line ();
> > +
> > +  if (fde->personality_id == 0 || fde->personality_id > 3)
> > +    {
> > +      as_bad (_("wrong argument to .cfi_personality_id"));
> > +      return;
> > +    }
> 
> No need for { ... return; }
> 
> > +static void
> > +output_compact_unwind_data (struct fde_entry *fde, int align)
> 
> Probably worth a comment here, since it wasn't obvious to me the "align" is
> the alignment of the end of the data rather than the start.
> 
> >  {
> > -  md_number_to_chars (frag_more (4), data, 4);
> > +  int data_size = fde->eh_data_size + 2;
> 
> Add a comment for the "2" (the personality encoding and the stop byte,
> right?)
> 
> > +  int align_padding;
> > +  int amask;
> > +  char *p;
> > +
> > +  fde->eh_loc = symbol_temp_new_now ();
> > +
> > +  p = frag_more (1);
> > +  if (fde->personality_id != 0)
> > +    *p = fde->personality_id;
> > +  else if (fde->per_encoding != DW_EH_PE_omit)
> > +    {
> > +      *p = 0;
> > +      emit_expr_encoded (&fde->personality, fde->per_encoding, FALSE);
> > +      data_size += encoding_size (fde->per_encoding);
> > +    }
> > +  else
> > +    *p = 1;
> > +
> > +  amask = (1 << align) - 1;
> > +  align_padding = ((data_size + amask) & ~amask) - data_size;
> 
> Simpler as:
> 
>   align_padding = -data_size & amask.
> 
> > +  align = get_absolute_expression ();  if (align > max_alignment)
> > +    {
> > +      align = max_alignment;
> > +      as_bad (_("Alignment too large: %d. assumed."), align);
> 
> No "." after %d.
> 
> > +  /* Open .gnu_extab section.  */
> > +  cfi_seg = get_cfi_seg (ccseg, ".gnu_extab",
> > +			 (SEC_ALLOC | SEC_LOAD | SEC_DATA
> > +			  | DWARF2_EH_FRAME_READ_ONLY),
> > +			 1);
> > +#ifdef md_fix_up_eh_frame
> > +  md_fix_up_eh_frame (cfi_seg);
> > +#else
> > +  (void) cfi_seg;
> > +#endif
> 
> md_fix_up_eh_frame doesn't seem appropriate for .gnu_extab.
> 
> > @@ -1576,29 +1889,43 @@ output_fde (struct fde_entry *fde, struct
> cie_entry *cie,
> >        TC_DWARF2_EMIT_OFFSET (cie->start_address, offset_size);
> >      }
> >
> > +  exp.X_op = O_symbol;
> > +  exp.X_add_symbol = fde->start_address;
> >    if (eh_frame)
> >      {
> > -      exp.X_op = O_subtract;
> > -      exp.X_add_number = 0;
> > +      if (tc_cfi_special_encoding (cie->fde_encoding))
> > +	{
> > +	  bfd_reloc_code_real_type code
> > +	    = tc_cfi_reloc_for_encoding (cie->fde_encoding);
> > +	  reloc_howto_type *howto = bfd_reloc_type_lookup (stdoutput,
> code);
> > +	  char *p = frag_more(4);
> 
> Space before "(".
> 
> > +	  md_number_to_chars (p, 0, 4);
> > +	  fix_new (frag_now, p - frag_now->fr_literal, 4, exp.X_add_symbol,
> > +		   exp.X_add_number, howto->pc_relative, code);
> 
> What's the significance of exp.X_add_number here?  If it was supposed to
> be initialised to 0 above then I think it would be easier to leave the "exp"
> stuff alone and just use fde->start_address and 0 directly in this fix_new call.
> Certainly...
> 
> >    else
> >      {
> > -      exp.X_op = O_symbol;
> > -      exp.X_add_symbol = fde->start_address;
> >        exp.X_add_number = 0;
> >        addr_size = DWARF2_ADDR_SIZE (stdoutput);
> >        emit_expr (&exp, addr_size);
> 
> ...separating the add_symbol and add_number feels odd.
> 
> > @@ -1614,24 +1941,7 @@ output_fde (struct fde_entry *fde, struct
> cie_entry *cie,
> >    if (eh_frame)
> >      out_uleb128 (augmentation_size);		/* Augmentation size.  */
> >
> > -  if (fde->lsda_encoding != DW_EH_PE_omit)
> > -    {
> > -      exp = fde->lsda;
> > -      if ((fde->lsda_encoding & 0x70) == DW_EH_PE_pcrel)
> > -	{
> > -#if CFI_DIFF_LSDA_OK
> > -	  exp.X_op = O_subtract;
> > -	  exp.X_op_symbol = symbol_temp_new_now ();
> > -	  emit_expr (&exp, augmentation_size);
> > -#elif defined (tc_cfi_emit_pcrel_expr)
> > -	  tc_cfi_emit_pcrel_expr (&exp, augmentation_size);
> > -#else
> > -	  abort ();
> > -#endif
> > -	}
> > -      else
> > -	emit_expr (&exp, augmentation_size);
> > -    }
> > +  emit_expr_encoded (&fde->lsda, cie->lsda_encoding, FALSE);
> 
> This emit_expr_encoded stuff was a nice clean-up, thanks.
> 
> > +  if (fde->eh_header_type == EH_COMPACT_INLINE)
> > +    {
> > +      p = frag_more (4);
> > +      /* Inline entries always use PR1.  */
> > +      *(p++) = 1;
> > +      memcpy(p, fde->eh_data, 3);
> 
> Space before "(".
> 
> > @@ -1888,7 +2223,17 @@ cfi_finish (void)
> >
> >  	  for (fde = all_fde_data; fde ; fde = fde->next)
> >  	    {
> > -	      if (SUPPORT_FRAME_LINKONCE)
> > +	      if ((fde->sections & CFI_EMIT_eh_frame) == 0)
> > +		continue;
> > +
> > +#if SUPPORT_COMPACT_EH
> > +	      if (fde->eh_header_type == EH_COMPACT_HAS_LSDA)
> > +		fde->eh_header_type = EH_COMPACT_LEGACY;
> > +
> > +	      if (fde->eh_header_type != EH_COMPACT_LEGACY)
> > +		continue;
> > +#endif
> 
> Please add a comment explaining this.
> 
> > +#if SUPPORT_COMPACT_EH
> > +      if (compact_eh)
> > +	{
> > +	  /* Create remaining out of line table entries.  */
> > +	  do
> > +	    {
> > +	      ccseg = NULL;
> > +	      seek_next_seg = 0;
> > +
> > +	      for (fde = all_fde_data; fde ; fde = fde->next)
> > +		{
> > +		  if ((fde->sections & CFI_EMIT_eh_frame) == 0)
> > +		    continue;
> > +
> > +		  if (fde->eh_header_type != EH_COMPACT_OUTLINE)
> > +		    continue;
> > +		  if (HANDLED (fde))
> > +		    continue;
> > +		  if (seek_next_seg && CUR_SEG (fde) != ccseg)
> > +		    {
> > +		      seek_next_seg = 2;
> > +		      continue;
> > +		    }
> > +		  if (!seek_next_seg)
> > +		    {
> > +		      ccseg = CUR_SEG (fde);
> > +		      /* Open .gnu_extab section.  */
> > +		      cfi_seg = get_cfi_seg (ccseg, ".gnu_extab",
> > +					     (SEC_ALLOC | SEC_LOAD |
> SEC_DATA
> > +					      |
> DWARF2_EH_FRAME_READ_ONLY),
> > +					     1);
> > +#ifdef md_fix_up_eh_frame
> > +		      md_fix_up_eh_frame (cfi_seg); #else
> > +		      (void) cfi_seg;
> > +#endif
> 
> As above, I don't think the md_fix_up_eh_frame is appropriate.
> 
> > +		      seek_next_seg = 1;
> > +		    }
> > +		  SET_HANDLED (fde, 1);
> > +
> > +		  frag_align (1, 0, 0);
> > +		  record_alignment (now_seg, 1);
> > +		  output_compact_unwind_data (fde, 1);
> > +		}
> > +	    }
> > +	  while (EH_FRAME_LINKONCE && seek_next_seg == 2);
> 
> The quadraticness seems a bit unfortunate, but I realise you're just following
> the existing structure.
> 
> > @@ -1958,6 +2394,9 @@ cfi_finish (void)
> >
> >  	  for (fde = all_fde_data; fde ; fde = fde->next)
> >  	    {
> > +	      if ((fde->sections & CFI_EMIT_debug_frame) == 0)
> > +		continue;
> > +
> >  	      if (SUPPORT_FRAME_LINKONCE)
> >  		{
> >  		  if (HANDLED (fde))
> 
> Please add a comment explaining why this is needed/correct.
> 
> Thanks,
> Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: compact-eh-2014-11-17.cl
Type: application/octet-stream
Size: 7576 bytes
Desc: compact-eh-2014-11-17.cl
URL: <https://sourceware.org/pipermail/binutils/attachments/20141117/e0cc152a/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: compact-eh-2014-11-17.patch
Type: application/octet-stream
Size: 131531 bytes
Desc: compact-eh-2014-11-17.patch
URL: <https://sourceware.org/pipermail/binutils/attachments/20141117/e0cc152a/attachment-0001.obj>


More information about the Binutils mailing list