RE: [Patch] Gas support for MIPS Compact EH

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

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Saturday, November 22, 2014 9:43 AM
> To: Moore, Catherine
> Cc: binutils@sourceware.org
> Subject: Re: [Patch] Gas support for MIPS Compact EH
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > 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.
> 
> OK, thanks.  I started by going through my previous comments and 
> looking at how you addressed them in the new patch.  I think there are 
> still some things that need to be sorted out.
> 
> When doing the next patch, could you reply to the main points and say 
> how you dealt with them?  That'd make it a lot easier to see how this is evolving.

Yes, I'll do that.  I'm sorry for the missing pieces of the last patch.  My rebase had some unexpected results that I didn't catch before I posted.
Your comment that additional tests would have caught those is correct.  I've added some link tests to help out with that.  I think that extending the readelf -u (dump unwind option) would be useful for additional tests, but I don't want to make that additional development a requirement for approval of this patch.  There are still one or two outstanding bits, but I'd like to defer those until I see your comments on the current patch.
> 
> >> -----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.
> 
> The new version of the patch doesn't have the .cfi_inline_lsda 
> documentation that the previous patch had.  Please add it back :-)  
> But like I say above, please also document how the data is terminated.

It's back.
> 
> >> >> 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.
> 
> The new hunk for this is:
> 
> > @@ -128,7 +241,15 @@ get_debugseg_name (segT seg, const char
> *base_name)
> >        dot = strchr (name + 1, '.');
> >
> >        if (!dollar && !dot)
> > -	name = "";
> > +	{
> > +	  /* Ensure that a uniquely named .eh_frame_entry section
> > +	     is created for each text section.  */
> > +	  if (compact_eh
> > +	       && !strcmp (base_name, ".eh_frame_entry")
> > +	       && strcmp (name, ".text") != 0)
> > +	    return concat (base_name, ".", name, NULL);
> > +	  name = "";
> > +	}
> 
> I don't think we want the compact_eh test here; just the base_name 
> check should be enough.  (Nit, sorry, but watch the indentation.
> One less space before "&&".)
> 
Done.
> >> >> > @@ -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.
> 
> The hunk for this is:
> 
> > diff --git a/gas/doc/internals.texi b/gas/doc/internals.texi index
> > cc4089b..9dd0bd0 100644
> > --- a/gas/doc/internals.texi
> > +++ b/gas/doc/internals.texi
> > @@ -1473,6 +1473,10 @@ completed, but before the relocations have
> been generated.
> >  If you define this macro, GAS will call it after the relocs have 
> > been generated.
> >
> > +@item tc_cfi_reloc_for_encoding
> > +@cindex tc_cfi_reloc_for_encoding
> > +You may define this macro to indicate whether a cfi encoding 
> > +requires a
> relocation.
> 
> That makes it sound like a boolean, whereas it returns a reloc.  
> Please also mention that it controls whether compact EH is supported.  
> The 80- character limit applies to documentation too.
> 

Done.

> >> >> > +  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?
> 
> Still not sure about this -- please could you clarify?  The context is 
> this part of
> dot_cfi_inline_lsda:
> 
> > +  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;
> > +    }
> > +
> > +#ifdef md_flush_pending_output
> > +  md_flush_pending_output ();
> > +#endif
> > +
> > +  align = get_absolute_expression ();  if (align > max_alignment)
> > +    {
> > +      align = max_alignment;
> > +      as_bad (_("Alignment too large: %d assumed."), align);
> > +    }
> > +  else if (align < 0)
> > +    {
> > +      as_warn (_("Alignment negative: 0 assumed."));
> > +      align = 0;
> > +    }
> > +
> > +  demand_empty_rest_of_line ();
> > +  ccseg = CUR_SEG (last_fde);
> > +  /* Open .gnu_extab section.  */
> > +  get_cfi_seg (ccseg, ".gnu_extab",
> > +	       (SEC_ALLOC | SEC_LOAD | SEC_DATA
> > +		| DWARF2_EH_FRAME_READ_ONLY),
> > +	       1);
> >
> > +  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);
> > +
> > +  last_fde = NULL;
> > +
> > +  return;
> 
> The first "if" statement allows the directive to be used for 
> EH_COMPACT_LEGACY, but what does the directive mean/do in that case?
> 

I've now added some commentary and extended the definition of the enumeration to include an EH_COMPACT_UNKNOWN value. 
The eh_header_type was never explicitly initialized; it's now initialized to EH_COMPACT_UNKNOWN.  The EH_COMPACT_LEGACY header type is then explicitly set when none of the COMPACT header type cases are discovered.

> >> > 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.
> 
> I see you've done this, thanks, but:
> 
> > +struct dwarf_eh_frame_hdr_info
> > +{
> > +  struct eh_frame_array_ent *array; };
> > +
> > +struct compact_eh_frame_hdr_info
> > +{
> > +  unsigned int allocated_entries;
> > +  /* eh_frame_entry_fragments.  */
> > +  asection **entries;
> > +};
> > +
> >  struct eh_frame_hdr_info
> >  {
> >    struct htab *cies;
> >    asection *hdr_sec;
> >    unsigned int fde_count, array_count;
> > -  struct eh_frame_array_ent *array;
> >    /* TRUE if .eh_frame_hdr should contain the sorted search table.
> >       We build it if we successfully read all .eh_frame input sections
> >       and recognize them.  */
> >    bfd_boolean table;
> > +  bfd_boolean frame_hdr_is_compact;  union
> > +    {
> > +      struct dwarf_eh_frame_hdr_info dwarf;
> > +      struct compact_eh_frame_hdr_info compact;
> > +    }
> > +  u;
> >  };
> >
> >  /* Enum used to identify target specific extensions to the 
> > elf_obj_tdata
> 
> ...aren't cies, fde_count, array_count and table specific to the DWARF 
> version too?  My point was that if we went for the union approach, the 
> only fields in the common structure should be those that are needed by 
> both formats.  It looks like that's just hdr_sec.
> 
> If moving all of them seems like too much work, I think we should go 
> for separate structures instead.

I moved them and kept the union.

> 
> >> > +	}
> >> > +
> >> > +      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.
> 
> I see you've added the earlier pass, thanks, but errors aren't always 
> reported back up.  The function:
> 
> > +/* Parse a .eh_frame_entry section.  Figure out which text section it
> > +   references.  */
> > +
> > +void
> > +_bfd_elf_parse_eh_frame_entry (struct bfd_link_info *info,
> > +			       asection *sec, struct elf_reloc_cookie *cookie) {
> > [...]
> > +fail:
> > +  (*_bfd_error_handler) (_("%B: failed to process 
> > +.eh_frame_entry"),
> > +sec->owner); }
> 
> doesn't tell the caller (and eventually the ld code) that a problem occured.
> The function ought to return a bfd_boolean success code, like its callers.
> Also, the error here seems to duplicate the eventual ld one:
> 
>     einfo (_("%P%F: Failed to parse EH frame entries.\n"));
> 
> without really providing any more information.
> 
> Don't shoot me, but it would probably be cleaner to make this an 
> ELF-specific pass and call it from elf32.em instead.  That'll avoid 
> having to do all the aout, bout, etc. stuff and would allow the ld 
> code to refer to ELFisms like .eh_frame_entry itself.
> 
Okay, I've now moved this to elf32.em.  I fixed the error handling as well.

> >> > +/* 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.
> 
> Note that the generic version of _bfd_elf_end_eh_frame_parsing was 
> removed in the meantime.  Your patch adds it back, but I can't see 
> where it gets called.  Unless I'm missing something, this suggests 
> that the tests don't exercise this code and that extra tests might be needed.

I've now added link-time tests and the call _bfd_elf_end_eh_frame_parsing has been restored.
> 
> >> > @@ -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.
> 
> It looks like you dealt with this by removing the error checking.
> I think we still want it, but it should be done separately and in a 
> context where the error can be propagated.

I'm deferring this for now.
> 
> >> > @@ -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.
> 
> Still present.
Fixed.
> 
> >> > +  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.
> 
> I see you've done this with:
> 
> > +  if (addr & 1)
> > +    {
> > +      (*_bfd_error_handler) (_("%B: %s invalid input section size"),
> > +			     sec->owner, sec->name);
> > +      return FALSE;
> > +    }
> > +  if (last_addr >= addr + sec->rawsize)
> > +    {
> > +      (*_bfd_error_handler) (_("%B: %s points past end of text section"),
> > +			     sec->owner, sec->name);
> > +      return FALSE;
> > +    }
> 
> but I think we want "bfd_set_error (bfd_error_bad_value);" too.

Done.
> 
> >> > +  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.
> 
> It doesn't look like you addressed this part.

It looked to me like those ports used 1?  In any case, I've added the hook, but I haven't updated the arm and c6x ports to use it.
> 
> >> 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.
> 
> As above, it looks like nothing ever calls 
> _bfd_elf_write_section_eh_frame_entry in the new version of the patch.
> If the tests don't pick that up then I think we need some more :-)
> 
Sorry about this missing bit.  More tests have been added and the call to this function has been restored.

> >> > @@ -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".
> 
> Not sure: why is this no longer in the patch?
Restored.
> 
> >> > @@ -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);
> 
> You changed this to:
> 
> > +  if (eh_frame && !eh_frame->gc_mark)
> > +    {
> > +      if (!_bfd_elf_gc_mark (info, eh_frame, gc_mark_hook))
> > +	ret = FALSE;
> > +    }
> 
> but the point was also that we don't want to try another mark once "ret"
> is already FALSE.  The "ret &&" part of the condition was important.
Another rebase problem.  Now fixed.
> 
> >> > @@ -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.
> 
> You changed this to:
> 
> > @@ -12171,7 +12256,9 @@ bfd_elf_gc_sections (bfd *abfd, struct 
> > bfd_link_info *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.
> > */
> > -  for (sub = info->input_bfds; sub != NULL; sub = sub->link.next)
> > +  for (sub = info->input_bfds;
> > +       info->eh_frame_hdr_type != COMPACT_EH_HDR && sub != NULL;
> > +       sub = sub->link.next)
> >      {
> >        asection *sec;
> >        struct elf_reloc_cookie cookie;
> 
> where info->eh_frame_hdr_type is an invariant.  I assume this was just 
> to avoid reformatting the block, but please use:
> 
>   if (info->eh_frame_hdr_type != COMPACT_EH_HDR)
>     for (...)
> 
> instead.  Fortunately the block's not that big. :-)

Done.
> 
> >> > 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.
> 
> Doesn't look like you've addressed this bit.

Deferring for now.
> 
> >> > @@ -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 ")".
> 
> As above, this part of the doc is no longer there.

Restored.
> 
> >> > @@ -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?
> 
> Has .cfi_epilogue_begin been dropped?

Yes.
> 
> >>
> >> > @@ -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.
> 
> The hunk for this is:
> 
> > +  /* We track the current segment in the cfi_insn_data struct and
> > +     the cfi_insn_data struct for Compact EH.  */  if (compact_eh)
> > +    return now_seg;
> 
> but the comment repeats "the cfi_insn_data struct".  TBH I'm still not 
> sure why/whether this is correct, but let's sort the other things out first.

Okay.
> 
> >> > -/* 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.
> 
> It doesn't look like you addressed this bit.
> 
> >> > +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.
> 
> I don't think you addressed this bit.  (In case it wasn't clear, I was 
> suggesting adding a function comment that says what the function does 
> and what its arguments are, etc.)

Done.
> 
> >> > +	  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.
> 
> This is now:
> 
> > @@ -1575,29 +1881,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;
> 
> (A)
> 
> >    if (eh_frame)
> >      {
> > -      exp.X_op = O_subtract;
> > -      exp.X_add_number = 0;
> > +      bfd_reloc_code_real_type code
> > +	= tc_cfi_reloc_for_encoding (cie->fde_encoding);
> > +      if (code !=  BFD_RELOC_NONE)
> > +	{
> > +	  reloc_howto_type *howto = bfd_reloc_type_lookup (stdoutput,
> code);
> > +	  char *p = frag_more (4);
> > +	  md_number_to_chars (p, 0, 4);
> > +	  fix_new (frag_now, p - frag_now->fr_literal, 4, fde->start_address,
> > +		   0, howto->pc_relative, code);
> > +	}
> > +      else
> > +	{
> > +	  exp.X_op = O_subtract;
> > +	  exp.X_add_number = 0;
> 
> (B)
> 
> >  #if CFI_DIFF_EXPR_OK
> > -      exp.X_add_symbol = fde->start_address;
> > -      exp.X_op_symbol = symbol_temp_new_now ();
> > -      emit_expr (&exp, DWARF2_FDE_RELOC_SIZE);	/* Code offset.  */
> > +	  exp.X_add_symbol = fde->start_address;
> > +	  exp.X_op_symbol = symbol_temp_new_now ();
> > +	  emit_expr (&exp, DWARF2_FDE_RELOC_SIZE);	/* Code
> offset.  */
> >  #else
> > -      exp.X_op = O_symbol;
> > -      exp.X_add_symbol = fde->start_address;
> > -#ifdef tc_cfi_emit_pcrel_expr
> > -      tc_cfi_emit_pcrel_expr (&exp, DWARF2_FDE_RELOC_SIZE);	 /*
> Code offset.  */
> > +	  exp.X_op = O_symbol;
> > +	  exp.X_add_symbol = fde->start_address;
> > +
> > +#if defined(tc_cfi_emit_pcrel_expr)
> > +	  tc_cfi_emit_pcrel_expr (&exp, DWARF2_FDE_RELOC_SIZE);	 /*
> Code offset.  */
> >  #else
> > -      emit_expr (&exp, DWARF2_FDE_RELOC_SIZE);	/* Code offset.  */
> > +	  emit_expr (&exp, DWARF2_FDE_RELOC_SIZE);	/* Code
> offset.  */
> >  #endif
> >  #endif
> > +	}
> >        addr_size = DWARF2_FDE_RELOC_SIZE;
> >      }
> >    else
> >      {
> > -      exp.X_op = O_symbol;
> > -      exp.X_add_symbol = fde->start_address;
> 
> (C)
> 
> >        exp.X_add_number = 0;
> >        addr_size = DWARF2_ADDR_SIZE (stdoutput);
> >        emit_expr (&exp, addr_size);
> 
> (C) is still hoisted to (A), but the only use of "exp" in the 
> "eh_frame" arm is
> (rightly) at (B), which overrides what (A) does.
> Please just drop (A) and leave the "else" arm unmodified.

Done.
> 
> >> > @@ -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.
> 
> I don't think you addressed this.
> 

Done.
> >> > @@ -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.
> 
> Same here.
> 
Deferred.