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]

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.

>> -----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.

>> >> 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 "&&".)

>> >> > @@ -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.

>> >> > +  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?

>> > 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.

>> > +	}
>> > +
>> > +      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.

>> > +/* 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.

>> > @@ -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.

>> > @@ -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.

>> > +  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.

>> > +  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.

>> 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 :-)

>> > @@ -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?

>> > @@ -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.

>> > @@ -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. :-)

>> > 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.

>> > @@ -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.

>> > @@ -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?

>> 
>> > @@ -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.

>> > -/* 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.)

>> > +	  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.

>> > @@ -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.

>> > @@ -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.

Thanks,
Richard


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