> -----Original Message----- > From: Moore, Catherine > Sent: Tuesday, January 05, 2016 8:53 AM > To: Richard Henderson; ja...@redhat.com > Cc: gcc-patches@gcc.gnu.org > Subject: FW: [RFA] Compact EH Patch [Ping * 2] > > Ping, Ping. > > -----Original Message----- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@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: ja...@redhat.com; Matthew Fortune > Subject: RE: [RFA] Compact EH Patch [Ping] > > > > > -----Original Message----- > > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > > ow...@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: ja...@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:r...@redhat.com] > > > Sent: Friday, September 18, 2015 3:25 PM > > > To: Moore, Catherine; gcc-patches@gcc.gnu.org > > > Cc: ja...@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. > >
