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

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

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

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

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

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

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

> +#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:

  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.

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.

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

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

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

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

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

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

> +/* Attempt to generate compact frame unwind encodings.  */
> +
> +void
> +mips_cfi_endproc (struct fde_entry *fde ATTRIBUTE_UNUSED)
> +{
> +  struct cfi_insn_data *insn;
> +  int reg;
> +  offsetT cfa_offset = 0;
> +  offsetT next_offset = 0;
> +  bfd_boolean reg_offset[MIPS_NUM_UNWIND_REGS];
> +  int cfa_reg = 29;
> +  bfd_byte opbuff[40];

Why 40?  As a maths exam might say, please show your working :-)

> +  /* Frame pointer must be callee caved.  */
> +  if (cfa_reg < 16
> +      || (cfa_reg > 23 && cfa_reg < 29)
> +      || cfa_reg > 30)
> +    return;
> +
> +  /* Can only increment stack pointer.  */
> +  if (cfa_offset < 0)
> +    return;
> +
> +  /* Stack must be aligned.  */
> +  if ((cfa_offset & (stack_align - 1)) != 0)
> +    return;
> +
> +  /* Registers must be contiguous.  There's only limited benefit in
> +     supprting more complex layouts, so For now we also require they
> +     be in order.  */
> +  next_offset = -gpr_size;
> +  for (i = MIPS_NUM_UNWIND_REGS - 1; i >= 0; i--)
> +    {
> +      if (reg_offset[i] == 0)
> +	continue;
> +
> +      if (reg_offset[i] != next_offset)
> +	return;
> +
> +      next_offset -= gpr_size;
> +    }

What happens after these early returns?  Are they user errors
(in which case we should report them), or do we fall back to something
else (in which case a boolean return value would probably make sense)?

> +  if (fde->lsda_encoding != DW_EH_PE_omit)
> +    fde->eh_header_type = EH_COMPACT_HAS_LSDA;
> +  else if (num_ops <= 3 && fde->per_encoding == DW_EH_PE_omit)
> +    fde->eh_header_type = EH_COMPACT_INLINE;
> +  else
> +    fde->eh_header_type = EH_COMPACT_OUTLINE;

Why does this part need to be in MIPS-specific code?

Thanks,
Richard


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