Re: [RFA] Compact EH Patch

[Richard Henderson <rth@redhat.com>]

> Index: libgcc/libgcc-std.ver.in
> ===================================================================
> --- libgcc/libgcc-std.ver.in	(revision 226409)
> +++ libgcc/libgcc-std.ver.in	(working copy)
> @@ -1918,6 +1918,7 @@ GCC_4.6.0 {
>    __morestack_current_segment
>    __morestack_initial_sp
>    __splitstack_find
> +  _Unwind_GetEhEncoding
>  }
>  
>  %inherit GCC_4.7.0 GCC_4.6.0
> @@ -1938,3 +1939,8 @@ GCC_4.7.0 {
>  %inherit GCC_4.8.0 GCC_4.7.0
>  GCC_4.8.0 {
>  }
> +
> +%inherit GCC_4.8.0 GCC_4.7.0
> +GCC_4.8.0 {
> +  __register_frame_info_header_bases
> +}

You can't push new symbols into old versions.  These have to go into the
version for the current gcc.

> Index: libstdc++-v3/config/abi/pre/gnu.ver
> ===================================================================
> --- libstdc++-v3/config/abi/pre/gnu.ver	(revision 226409)
> +++ libstdc++-v3/config/abi/pre/gnu.ver	(working copy)
> @@ -1909,6 +1909,7 @@ CXXABI_1.3 {
>      __gxx_personality_v0;
>      __gxx_personality_sj0;
>      __gxx_personality_seh0;
> +    __gnu_compact_pr2;
>      __dynamic_cast;
>  
>      # *_type_info classes, ctor and dtor
> Index: libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver
> ===================================================================
> --- libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver	(revision 226409)
> +++ libstdc++-v3/config/abi/pre/gnu-versioned-namespace.ver	(working copy)
> @@ -200,6 +200,7 @@ CXXABI_2.0 {
>      __cxa_vec_new;
>      __gxx_personality_v0;
>      __gxx_personality_sj0;
> +    __gnu_compact_pr2;
>      __dynamic_cast;
>  
>      # std::exception_ptr

Likewise.

> +  if (data.type != CET_not_found)
> +    return data.type;
> +
> +  return CET_not_found;

Return data.type unconditionally.

> +++ libgcc/unwind-pe.h	(working copy)
> @@ -44,6 +44,7 @@
>  #define DW_EH_PE_udata2         0x02
>  #define DW_EH_PE_udata4         0x03
>  #define DW_EH_PE_udata8         0x04
> +#define DW_EH_PE_udata1         0x05
>  #define DW_EH_PE_sleb128        0x09
>  #define DW_EH_PE_sdata2         0x0A
>  #define DW_EH_PE_sdata4         0x0B

If we're going to add udata1, we might as well add sdata1 for consistency.

> @@ -184,6 +187,7 @@ read_encoded_value_with_base (unsigned char encodi
>    union unaligned
>      {
>        void *ptr;
> +      unsigned u1 __attribute__ ((mode (QI)));
>        unsigned u2 __attribute__ ((mode (HI)));
>        unsigned u4 __attribute__ ((mode (SI)));
>        unsigned u8 __attribute__ ((mode (DI)));

This is silly.  Access to a single byte never requires alignment help from the
compiler...

> +	case DW_EH_PE_udata1:
> +	  result = u->u1;
> +	  p += 1;
> +	  break;

    result = *p;

> +  read_encoded_value_with_base (DW_EH_PE_absptr | DW_EH_PE_udata4, 0,
> +				hdr + 4, &nrec);

Err... that encoding type makes no sense: absptr is "pointer size".  Combining
that with an explicit size, like udata4, means that udata4 wins.  So this is
the same as simply writing DW_EH_PE_udata4.

At which point you're loading a 4 byte unsigned quantity from an aligned
address.  You might as well use *(uint32_t *)(hdr + 4).

> +__register_frame_info_header_bases (const void *begin, struct object *ob,
> +				    void *tbase, void *dbase)
> +{
> +  /* Only register compact EH frame headers.  */
> +  if (begin == NULL || *(const char *) begin != 2)
> +    return;

Check for no entries before registering?

> +extern char __GNU_EH_FRAME_HDR[] TARGET_ATTRIBUTE_WEAK;

Don't you have some guaranteed alignment for this table?
That perhaps ought to be seen in either the type or an attribute.

+      if (op < 0x40)
+      else if (op < 0x48)
+      else if (op < 0x50)
+      else if (op < 0x58)
+      else if (op == 0x58)
+      else if (op == 0x59)
+      else if (op == 0x5a)
+      else if (op == 0x5b)
+      else if (op == 0x5c)
+      else if (op == 0x5d)
+      else if (op == 0x5e)
+      else if (op == 0x5f)
+      else if (op >= 0x60 && op < 0x6c)

Better as a switch statement surely, using the gcc case x ... y: extension.

> +static _Unwind_Reason_Code
> +uw_frame_state_compact (struct _Unwind_Context *context,
> +			_Unwind_FrameState *fs,
> +			enum compact_entry_type entry_type,
> +		       	struct compact_eh_bases *bases)
> +{
> +  const unsigned char *p;
> +  unsigned int pr_index;
> +  _Unwind_Ptr personality;
> +  unsigned char buf[4];
> +  _Unwind_Reason_Code rc;
> +
> +  p = bases->entry;
> +  pr_index = *(p++);
> +  switch (pr_index) {
> +  case 0:
> +      p = read_encoded_value (context, bases->eh_encoding, p, &personality);
> +      fs->personality = (_Unwind_Personality_Fn) personality;
> +      break;
> +  case 1:
> +      fs->personality = __gnu_compact_pr1;
> +      break;
> +  case 2:
> +      fs->personality = __gnu_compact_pr2;
> +      break;
> +  default:
> +      fs->personality = NULL;
> +  }

This is the aspect of this spec about which I am least keen.  The existing
method whereby the personality function is explicit in each object file means
that we've got automatic version control on data that is private to the object
file.

That is, if we should ever change the format of the LSDA -- as in fact you are
doing here -- then all we need do is change __gcc_personality_v0 to
__gcc_personality_v1, and all is well.  One can mix and match object files from
different compiler versions and all that is required for correctness is that
the runtime libraries must continue to provide all previous versions.

What you're doing here doesn't allow the format of index {1,2,3} to *ever*
change.  You've fixed it forever, barring abandoning those indices and always
using index 0.

I know that the ARM EH format made exactly the same mistake, but let's see if
we can find some way of not replicating it, eh?


> @@ -206,4 +206,14 @@ _Unwind_SetIP (struct _Unwind_Context *context, _U
>    return __libunwind_Unwind_SetIP (context, val);
>  }
>  symver (_Unwind_SetIP, GCC_3.0);
> +
> +extern unsigned char __libunwind_Unwind_GetEhEncoding
> +  (struct _Unwind_Context *);
> +
> +unsigned char
> +_Unwind_GetEhEncoding (struct _Unwind_Context *context)
> +{
> +  return __libunwind_Unwind_GetEhEncoding (context, val);
> +}
> +symver (_Unwind_GetEhEncoding, GCC_3.0);
>  #endif

There is no such __libunwind symbol, is there?

> +mcompact-eh
> +Target Var(TARGET_COMPACT_EH) Init(1)
> +Use compact exception unwinding tables.

This ought to be automatically disabled with -fasynchronous-unwind-tables.

>  	  /* "Save" $sp in itself so we don't use the fake CFA.
>  	     This is: DW_CFA_val_expression r29, { DW_OP_reg29 }.  */
> -	  fprintf (asm_out_file, "\t.cfi_escape 0x16,29,1,0x6d\n");
> +	  if (dwarf2out_do_frame ()
> +	      && (flag_unwind_tables || flag_exceptions))
> +	    /* "Save" $sp in itself so we don't use the fake CFA.
> +	       This is: DW_CFA_val_expression r29, { DW_OP_reg29 }.  */
> +	    fprintf (asm_out_file, "\t.cfi_fde_data 0x5b\n");
> +	  else
> +	    fprintf (asm_out_file, "\t.cfi_escape 0x16,29,1,0x6d\n");

The commentary appears to be messed up?

> +  /* Place eh-related data in sdata so that we can use a gprel32 reloc
> +     to access it.  */

I thought sdata was reserved for 16-bit offsets.  Have things changed over the
years for the mips target?


r~