This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
RE: [Patch] Gas support for MIPS Compact EH
- From: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- To: "Moore, Catherine" <Catherine_Moore at mentor dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Tue, 5 May 2015 10:04:42 +0000
- Subject: RE: [Patch] Gas support for MIPS Compact EH
- Authentication-results: sourceware.org; auth=none
- References: <FD3DCEAC5B03E9408544A1E416F11242F8FC5972 at NA-MBX-01 dot mgc dot mentorg dot com> <87k3me9jia dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F11242012EAC2AE0 at NA-MBX-01 dot mgc dot mentorg dot com> <8738jt5zt1 dot fsf at talisman dot default> <FD3DCEAC5B03E9408544A1E416F112420189115903 at NA-MBX-01 dot mgc dot mentorg dot com> <871tov9tzs dot fsf at googlemail dot com> <FD3DCEAC5B03E9408544A1E416F112420189151B67 at NA-MBX-04 dot mgc dot mentorg dot com> <FD3DCEAC5B03E9408544A1E416F112420189E70AF2 at NA-MBX-04 dot mgc dot mentorg dot com> <FD3DCEAC5B03E9408544A1E416F11242018EB214D9 at NA-MBX-04 dot mgc dot mentorg dot com>
Hi Catherine,
For what it's worth I'm reading the spec for this for the first time
and will aim to look through the patch. I wonât be able to approve it
but it may help give some assurance of the patch for other reviewers.
Thanks,
Matthew
> -----Original Message-----
> From: binutils-owner@sourceware.org [mailto:binutils-
> owner@sourceware.org] On Behalf Of Moore, Catherine
> Sent: 01 May 2015 14:55
> To: binutils@sourceware.org
> Subject: 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.