This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

FW: [Patch] Gas support for MIPS Compact EH


Ping.  I'd really like to move forward with this patch.   A large portion of the patch is not MIPS-specific, although there are some MIPS-specific pieces.
Would someone be able to review this?
Thanks,
Catherine

-----Original Message-----
From: binutils-owner@sourceware.org [mailto:binutils-owner@sourceware.org] On Behalf Of Moore, Catherine
Sent: Thursday, April 16, 2015 9:28 AM
To: Richard Sandiford
Cc: binutils@sourceware.org
Subject: RE: [Patch] Gas support for MIPS Compact EH

Hi Richard,
Is this patch something that you are going to be able to handle sometime soon?
If not, perhaps one of the other maintainers would be able to review this.
Thanks,
Catherine

> -----Original Message-----
> From: binutils-owner@sourceware.org [mailto:binutils- 
> owner@sourceware.org] On Behalf Of Moore, Catherine
> Sent: Sunday, February 08, 2015 12:00 PM
> To: Richard Sandiford
> Cc: binutils@sourceware.org
> Subject: RE: [Patch] Gas support for MIPS Compact EH
> 
> Sorry, forgot the patch in last message.
> 
> > -----Original Message-----
> > From: Moore, Catherine
> > Sent: Sunday, February 08, 2015 11:59 AM
> > To: 'Richard Sandiford'
> > Cc: binutils@sourceware.org
> > Subject: RE: [Patch] Gas support for MIPS Compact EH
> >
> > > -----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.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]