FW: [RFA] Compact EH Patch [Ping * 2]

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

Ping, Ping.

-----Original Message-----
From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Moore, Catherine
Sent: Monday, December 21, 2015 9:22 AM
To: Richard Henderson; gcc-patches@gcc.gnu.org
Cc: jason@redhat.com; Matthew Fortune
Subject: RE: [RFA] Compact EH Patch [Ping]



> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- 
> owner@gcc.gnu.org] On Behalf Of Moore, Catherine
> Sent: Sunday, December 13, 2015 5:12 PM
> To: Richard Henderson; gcc-patches@gcc.gnu.org
> Cc: jason@redhat.com; Matthew Fortune
> Subject: RE: [RFA] Compact EH Patch
> 
> I've now updated the patch.  I'd like to get this in GCC 6.0; please 
> let me know if that is possible.
> I've also updated the spec (https://github.com/MentorEmbedded/cxx-
> abi/MIPSCompactEH.pdf).
>  rth commented that there was no versioning info provision for the 
> personality routines.  The now spec identifies a couple of bits in the 
> header that can be used for that.  A couple of new unwind opcodes were 
> also added.
> 
> With the exception of Matthew's comment about disabling Compact EH for 
> those ABIs that don't support it, I have not addressed the 
> MIPS-specific comments.  I will post a follow-on patch next week to take care of that.
> There is also a problem with N64 ABI support that requires a follow-on 
> binutils patch.  I have disabled Compact EH for N64 on Linux in this 
> version of the patch.  I plan to re-enable it after binutils is updated.
> 
> Okay to commit?
> 
> Thanks,
> Catherine
> 
> 
> > -----Original Message-----
> > From: Richard Henderson [mailto:rth@redhat.com]
> > Sent: Friday, September 18, 2015 3:25 PM
> > To: Moore, Catherine; gcc-patches@gcc.gnu.org
> > Cc: jason@redhat.com; Matthew Fortune
> > Subject: Re: [RFA] Compact EH Patch
> >
> > > 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.
> 
> Done.
> >
> > > 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
> 
> Done.
> 
> > > 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
> >
> Jonathan says that CXXABI_2.0 is correct here.
> >
> > > +  if (data.type != CET_not_found)
> > > +    return data.type;
> > > +
> > > +  return CET_not_found;
> >
> > Return data.type unconditionally.
> 
> Done.
> >
> > > +++ 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.
> 
> Done.
> >
> > > @@ -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;
> 
> Done.
> 
> >
> > > +  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).
> >
> 
> Done.
> > > +__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?
> 
> Done.
> >
> > > +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.
> 
> Done.
> >
> > > +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?
> 
> 
> This has been addressed with a specification update to allocate some 
> additional bits for versioning.
> >
> >
> > > @@ -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?
> 
> Removed.
> >
> > > +mcompact-eh
> > > +Target Var(TARGET_COMPACT_EH) Init(1) Use compact exception
> > unwinding
> > > +tables.
> >
> > This ought to be automatically disabled with -fasynchronous-unwind-
> tables.
> 
> We don't disable it, but we emit dwarf-based unwind information.  The 
> compact header knows that it's DWARF and processes it accordingly.
> See Section 10 in the spec -- we have compact-inline, compact-outline 
> and legacy-style unwind info.
> 
> >
> > >  	  /* "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?
> 
> Fixed.
>