Re: [Patch] Gas support for MIPS Compact EH

[Richard Sandiford <rdsandiford at googlemail dot com>]

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