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