> -----Original Message-----
> From: Richard Henderson [mailto:r...@redhat.com]
> Subject: Re: [RFA] Compact EH Patch
> 

Richard, Thanks for the patch review.  
Matthew, Would you please take a look at the MIPS-specific pieces before I 
resubmit the patch?
There will be a small change to the data encoding for the MIPS-Linux toolchain 
to fix a problem that we found in the mabi=64 libraries.
 A follow-on patch to binutils will also be required.  I'll do an update to 
this patch, soon, including any changes for the MIPS backend.

Please see the embedded comments and a couple of questions.

Thanks,
Catherine

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

Yep, okay.  I'll fix these in the next version.
> 
> Likewise.
> 
> > +  if (data.type != CET_not_found)
> > +    return data.type;
> > +
> > +  return CET_not_found;
> 
> Return data.type unconditionally.

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

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

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

Ok, here as well.

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

That's reasonable.

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

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

I'm not 100% sure what you are suggesting here.  Would you like to see 
__gnu_compact_pr2_v0,  etc?  
Are you also saying that you want to see a undefined reference to the 
personality routine in the object file?

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

No there isn't.    I'll remove this.
> 
> > +mcompact-eh
> > +Target Var(TARGET_COMPACT_EH) Init(1) Use compact exception
> unwinding
> > +tables.
> 
> This ought to be automatically disabled with -fasynchronous-unwind-tables.

No problem, I can do this.

> 
> >       /* "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?
> 
Yes, it is.


Reply via email to