RE: [Patch] Gas support for MIPS Compact EH

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

Hi Richard,

I finally got some time to work on fixing up (with some help from Bernd) the binutils submission for compact EH.   Hopefully,  the review comments have been addressed.
The current patch has been expanded to include all of binutils. 

As a reminder, the spec is available here:
https://github.com/MentorEmbedded/cxx-abi/blob/master/MIPSCompactEH.pdf

Specific comments related to your review are embedded.

Thanks,
Catherine

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Saturday, June 01, 2013 7:08 AM
> To: Moore, Catherine
> Cc: binutils@sourceware.org
> Subject: Re: [Patch] Gas support for MIPS Compact EH
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> > Index: doc/as.texinfo
> >
> ==========================================================
> =========
> > RCS file: /cvs/src/src/gas/doc/as.texinfo,v retrieving revision 1.266
> > diff -p -u -r1.266 as.texinfo
> > --- doc/as.texinfo	29 Apr 2013 13:38:58 -0000	1.266
> > +++ doc/as.texinfo	31 May 2013 17:24:20 -0000
> > @@ -4479,6 +4479,9 @@ if @var{section_list} is @code{.debug_fr  To
> > emit both use @code{.eh_frame, .debug_frame}.  The default if this
> > directive is not used is @code{.cfi_sections .eh_frame}.
> >
> > +On targets that support compact unwinding tables these can be
> > +generated by specifying @code{.eh_frame_header} instead of
> @code{.eh_frame}.
> 
> Should this be .eh_frame_entry instead of .eh_frame_header?

Yes, now fixed.

> 
> > @@ -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.
> 
> "If a compact encoding is being used" seems redundant, since it comes just
> after the bit saying "only valid when generating compact EH frames".
> 
> There need to be tests for all of this. 

Tests are now included in the patch.

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

> 
> > @@ -161,6 +177,9 @@ alloc_debugseg_item (segT seg, int subse  static
> > segT  is_now_linkonce_segment (void)  {
> > +  if (compact_eh)
> > +    return now_seg;
> > +
> >    if ((bfd_get_section_flags (stdoutput, now_seg)
> >         & (SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD
> >  	  | SEC_LINK_DUPLICATES_ONE_ONLY |
> SEC_LINK_DUPLICATES_SAME_SIZE
> 
> Why is this change needed?
> 
> > @@ -180,7 +199,11 @@ make_debug_seg (segT cseg, char *name, i
> >    segT r;
> >    flagword flags;
> >
> > +#ifdef tc_make_debug_seg
> > +  r = tc_make_debug_seg (cseg, name); #else
> >    r = subseg_new (name, 0);
> > +#endif
> 
> Why is this change needed?  And what does the new hook do?  It should be
> documented in internals.texi.
> 
> Do we really want to change the behaviour for traditional DWARF EH too?
> 

It looks like this was intended to make .group sections so that text, extab and eh_frame entries are only deleted together.  This is now removed and this new patch add some linker smarts to handle things.

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

> > @@ -1042,6 +1072,13 @@ dot_cfi_sections (int ignored ATTRIBUTE_
> >  	  sections |= CFI_EMIT_eh_frame;
> >  	else if (strncmp (name, ".debug_frame", sizeof ".debug_frame") ==
> 0)
> >  	  sections |= CFI_EMIT_debug_frame;
> > +#if SUPPORT_COMPACT_EH
> > +	else if (strncmp (name, ".eh_frame_entry", sizeof
> ".eh_frame_entry") == 0)
> > +	  {
> > +	    compact_eh = TRUE;
> > +	    sections |= CFI_EMIT_eh_frame | CFI_EMIT_target;
> > +	  }
> > +#endif
> >  #ifdef tc_cfi_section_name
> >  	else if (strcmp (name, tc_cfi_section_name) == 0)
> >  	  sections |= CFI_EMIT_target;
> 
> I think there needs to be more error checking here.  Do we support mixtures
> of DWARF and new-format EH info in the same .s file?
No, we don't support mixing the two formats in the same .s file.
Error checking has now been added.

> If not, we should check for it, including making sure that later .cfi_sections
> don't contradict earlier ones.  (At the moment, the last one wins, except for
> the CFI_EMIT_target handling.) If we do support mixtures, shouldn't
> something clear compat_eh, since compat_eh is checked in parsing
> contexts?
> 
> Why is the CFI_EMIT_target needed?
Sttrictly speaking it isn't, but having a different value was useful in the error checking.

> > @@ -1125,16 +1162,147 @@ dot_cfi_endproc (int ignored ATTRIBUTE_U
> >        return;
> >      }
> >
> > -  fde = frchain_now->frch_cfi_data->cur_fde_data;
> > +  last_fde = frchain_now->frch_cfi_data->cur_fde_data;
> >
> >    cfi_end_fde (symbol_temp_new_now ());
> >
> >    demand_empty_rest_of_line ();
> >
> > -  if ((cfi_sections & CFI_EMIT_target) != 0)
> > -    tc_cfi_endproc (fde);
> > +  if ((cfi_sections & CFI_EMIT_target) != 0
> > +      || (compact_eh && (cfi_sections & CFI_EMIT_eh_frame) != 0))
> > +    tc_cfi_endproc (last_fde);
> >  }
> 
> I think it'd be better to use a separate hook, especially if the idea is that other
> targets could use compact EH info.  Targets might then need a "legacy"
> tc_cfi_endproc as well as a "new" hook.
> 
Now changed to use .cfi_fde_data.

> > +#if SUPPORT_COMPACT_EH
> > +static void
> > +output_compact_unwind_data (struct fde_entry *fde, int align) {
> > +  int data_size = fde->eh_data_size + 2;
> > +  int amask;
> > +  char *p;
> > +  char *end;
> > +
> > +  fde->eh_loc = symbol_temp_new_now ();  if (fde->per_encoding !=
> > + DW_EH_PE_omit)
> > +    data_size += 4;
> 
> This 4 shouldn't be hardcoded.  Hopefully an unconditional:

Now fixed.

> 
>   data_size += encoding_size (fde->per_encoding);
> 
> would work; if not, please extend encoding_size.  We then have:
> 
> > +  if (fde->per_encoding != DW_EH_PE_omit)
> > +    {
> > +      *(p++) = 0;
> > +      md_number_to_chars (p, 0, 4);
> > +      tc_cfi_fix_eh_ref (p, &fde->personality);
> > +      p += 4;
> 
> where tc_cfi_fix_eh_ref emits an R_MIPS_EH relocation regardless of which
> personality encoding is used.  AIUI, the encoding must be one of the
> "special" ones allowed by tc_cfi_special_encoding, so we should check for
> that.
> 
> The tc_cfi_fix_eh_ref and tc_cfi_emit_expr hooks don't seem very
> consistent; the former relies on the caller to clear the bytes, whereas the
> latter is supposed to do it itself.

All fixed, now using a hook to return a reloc and eliminated the use of R_MIPS_EH from the assembler.
> 
> Neither of the hooks really seem to be doing anything target-specific except
> for the all-important job of picking a reloc.  I think it would be cleaner to have
> a hook that returns the reloc instead.
> In the case of tc_cfi_emit_expr, this could be done by making
> tc_cfi_special_encoding return the reloc for an encoding, or
> BFD_RELOC_NONE for unsupported encodings.
> 
> Also, this is more of a design point, but I find the handling of the personality
> encoding and R_MIPS_EH handling a bit confusing.  To quote:
> 
> ---------------------------------------------------------------------
> 11 Relocations
> 
> A new static relocation, R_MIPS_EH, is defined. The semantics of this
> relocation depend on whether static or dynamic linking is provided.
> 
> A GNU extension using relocation number 249 shall be used. The relocation
> address need not be naturally aligned.
> 
> 11.1 Static code
> 
> For static code generation, the calculation is the same as an
> R_MIPS_REL32 relocation.
> 
> At runtime, the following expression provides the relocated value, if 'ptr'
> points to the relocation location.
> 
> â (ptrdiff_t)ptr + *(int32_t __attribute__((packed))*)ptr
> 
> [...]
> 
> 11.2 PIC code
> 
> For PIC code generation, a 32- or 64-bit GOT-table entry must be allocated to
> refer to the (dynamically resolved) target address. Once the GOT entry has
> been allocated, the static calculation is as for an
> R_MIPS_GPREL32 relocation (except that the symbol is externally visible).
> The GOT- slot has an associated R_MIPS_{32,64} dynamic relocation emitted
> â and that will of course be at a naturally aligned location
> 
> At runtime, the following expression provides the relocated value, if 'ptr'
> points to the relocation location, and 'gp' is the global pointer
> value:
> 
> â *(ptrdiff_t *)((ptrdiff_t)gp + *(int32_t __attribute__((packed)) *)ptr)
> 
> [...]
> ---------------------------------------------------------------------
> 
> So as I understand the first sentence, it's actually the linker that decides
> whether R_MIPS_EH relocations act as direct PC-relative references (11.1) or
> as indirect datarel references (11.2).
> It's therefore the linker that decides what DWARF encoding R_MIPS_EH
> fields use.  The linker then records that choice in the .eh_frame_hdr.  Is that
> right?
> 
> If so, the assembler seems to be associating R_MIPS_EH with specific DWARF
> encodings, even though the interpretation of R_MIPS_EH isn't known at that
> stage.  I.e. the code reads:
> 
> #define tc_cfi_special_encoding(e) \
>   ((e) == (DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect) \
>    || (e) == (DW_EH_PE_sdata4 | DW_EH_PE_pcrel))
> 
> void
> mips_cfi_emit_expr (expressionS *exp, int encoding) {
>   char *p;
> 
>   p = frag_more(4);
>   md_number_to_chars (p, 0, 4);
>   if ((encoding & 0x70) == DW_EH_PE_datarel)
>     mips_cfi_fix_eh_ref (p, exp);
>   else
>     {
>       fix_new (frag_now, p - frag_now->fr_literal, 4, exp->X_add_symbol,
> 	       exp->X_add_number, TRUE, BFD_RELOC_32_PCREL);
>     }
> }
> 
> where mips_cfi_fix_eh_ref emits an R_MIPS_EH.  So the code appears to be
> using R_MIPS_EH for the DWARF encoding:
> 
>     DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect
> 
> even though there's no guarantee that the final value will be either datarel or
> indirect.
> 
> Or, to put it another way, why are we making the choice between
> R_MIPS_PC32 and R_MIPS_EH at assembly time in mips_cfi_emit_expr, but
> not in mips_cfi_fix_eh_ref, even though the patch appears to allow the
> same two encodings in both casees?
> 
> > +  memcpy (p, fde->eh_data, fde->eh_data_size);  p +=
> > + fde->eh_data_size;  while (p != end)
> > +    *(p++) = 0x5f;
> > +
> > +  *(p++) = 0x5c;
> 
> Please use something more mnemonic than 0x5f and 0x5c.  Are these the
> "Spare" and "PC = VR[31]; Finish" encodings from page 16?
> If so, they seem target-specific, so should probably use tc_* #defines.

Now fixed.
> 
> > +static void
> > +dot_cfi_inline_lsda (int ignored ATTRIBUTE_UNUSED) {
> > +  segT cfi_seg, ccseg;
> > +  int align;
> > +  long max_alignment = 28;
> > +
> > +  if (!last_fde)
> > +    {
> > +      as_bad (_("unexpected .cfi_inline_lsda"));
> > +      ignore_rest_of_line ();
> > +      return;
> > +    }
> > +
> > +  if (!compact_eh
> > +      || (last_fde->sections & CFI_EMIT_eh_frame) == 0
> > +      || (last_fde->eh_header_type != EH_COMPACT_LEGACY
> > +	  && last_fde->eh_header_type != EH_COMPACT_HAS_LSDA))
> > +
> > +    {
> > +      as_bad (_(".cfi_inline_lsda not valid for this frame"));
> > +      ignore_rest_of_line ();
> > +      return;
> > +    }
> 
> Excess blank line.  The first two lines of the if are clear enough, but what is
> the eh_header_type condition doing?  It looks like one of the cases it catches
> is where two LSDAs are specified for the same frame.
> If so, I think a better error message should be given in that case.

All fixed including a test case for the error message.
> 
> > +  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.
> 
> > @@ -1479,7 +1647,11 @@ output_cie (struct cie_entry *cie, bfd_b
> >  	  offsetT size = encoding_size (cie->per_encoding);
> >  	  out_one (cie->per_encoding);
> >  	  exp = cie->personality;
> > -	  if ((cie->per_encoding & 0x70) == DW_EH_PE_pcrel)
> > +	  if (tc_cfi_special_encoding (cie->per_encoding))
> > +	    {
> > +	      tc_cfi_emit_expr (&exp, cie->per_encoding);
> > +	    }
> 
> Excess braces.

Should be all fixed now.
> 
> > @@ -1617,7 +1804,11 @@ output_fde (struct fde_entry *fde, struc
> >    if (fde->lsda_encoding != DW_EH_PE_omit)
> >      {
> >        exp = fde->lsda;
> > -      if ((fde->lsda_encoding & 0x70) == DW_EH_PE_pcrel)
> > +      if (tc_cfi_special_encoding (cie->lsda_encoding))
> > +	{
> > +	  tc_cfi_emit_expr (&exp, cie->lsda_encoding);
> > +	}
> 
> Same here.
> 
> > +#if SUPPORT_COMPACT_EH
> > +static void
> > +cfi_emit_eh_header (symbolS *sym, bfd_vma addend)
> >  {
> > -  if (SUPPORT_FRAME_LINKONCE)
> > +  expressionS exp;
> > +
> > +  exp.X_add_number = addend;
> > +  exp.X_add_symbol = sym;
> > +  if (tc_cfi_special_encoding (DW_EH_PE_sdata4 | DW_EH_PE_pcrel))
> >      {
> > -      struct dwcfi_seg_list *l;
> > +      tc_cfi_emit_expr (&exp, DW_EH_PE_sdata4 | DW_EH_PE_pcrel);
> > +    }
> 
> Here too.
> 
> > +		      /* Open .eh_frame section.  */
> > +		      cfi_seg = get_cfi_seg (ccseg, ".gnu_extab",
> > +					     (SEC_ALLOC | SEC_LOAD |
> SEC_DATA
> > +					      |
> DWARF2_EH_FRAME_READ_ONLY),
> > +					     1);
> 
> Pasto, this isn't .eh_frame.

Fixed.
> 
> > @@ -15712,7 +15712,8 @@ md_apply_fix (fixS *fixP, valueT *valP,
> >  	      || fixP->fx_r_type == BFD_RELOC_MICROMIPS_SUB
> >  	      || fixP->fx_r_type == BFD_RELOC_VTABLE_INHERIT
> >  	      || fixP->fx_r_type == BFD_RELOC_VTABLE_ENTRY
> > -	      || fixP->fx_r_type == BFD_RELOC_MIPS_TLS_DTPREL64);
> > +	      || fixP->fx_r_type == BFD_RELOC_MIPS_TLS_DTPREL64
> > +	      || fixP->fx_r_type == BFD_RELOC_NONE);
> >
> >    buf = fixP->fx_frag->fr_literal + fixP->fx_where;
> >
> > @@ -15951,6 +15952,7 @@ md_apply_fix (fixS *fixP, valueT *valP,
> >          S_SET_WEAK (fixP->fx_addsy);
> >        break;
> >
> > +    case BFD_RELOC_NONE:
> >      case BFD_RELOC_VTABLE_ENTRY:
> >        fixP->fx_done = 0;
> >        break;
> 
> Why are these two hunks needed?

Now reverted.
> 
> > @@ -19793,3 +19795,289 @@ tc_mips_regname_to_dw2regnum (char
> *regn
> >
> >    return regnum;
> >  }
> > +
> > +#if defined (OBJ_ELF)
> > +segT
> > +mips_make_debug_seg (segT cseg, char *name) {
> > +  const char *group_name = NULL;
> > +  int linkonce;
> > +  int flags = 0;
> > +
> > +  if (cseg && (cseg->flags & SEC_LINK_ONCE) != 0)
> > +    {
> > +      group_name = elf_group_name (cseg);
> > +      if (group_name == NULL)
> > +	{
> > +	  as_bad (_("Group section `%s' has no group signature"),
> > +		  segment_name (cseg));
> > +	}
> > +      flags |= SHF_GROUP;
> > +      linkonce = 1;
> > +    }
> > +  obj_elf_change_section (name, SHT_PROGBITS, flags, 0, group_name,
> > +linkonce, 0);
> > +  return now_seg;
> > +}
> > +
> 
> This function doesn't seem to contain any MIPS-specific code.
> If a hook really is needed, then I think this implementation belongs in obj-
> elf.c rather than tc-mips.c.

Now removed in favor of linker smarts.
> 
> > +/* Attempt to generate compact frame unwind encodings.  */
> > +
> > +void
> > +mips_cfi_endproc (struct fde_entry *fde ATTRIBUTE_UNUSED) {

All remaining comments are against this function which was removed.  The compact opcodes are
Now generated by gcc using a .cfi_fde_data directive.

Attachment: compact-eh.cl
Description: compact-eh.cl

Attachment: compact-eh.patch
Description: compact-eh.patch