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] | |
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.
Attachment:
compact-eh-2015-02-08.cl
Description: compact-eh-2015-02-08.cl
Attachment:
compact-eh-2015-02-08.patch
Description: compact-eh-2015-02-08.patch
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |