From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19439 invoked by alias); 1 Jun 2013 11:07:48 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 19411 invoked by uid 89); 1 Jun 2013 11:07:47 -0000 X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,SPF_PASS,TW_CP,TW_WC autolearn=ham version=3.3.1 Received: from mail-wi0-f180.google.com (HELO mail-wi0-f180.google.com) (209.85.212.180) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sat, 01 Jun 2013 11:07:46 +0000 Received: by mail-wi0-f180.google.com with SMTP id hn14so1383814wib.13 for ; Sat, 01 Jun 2013 04:07:43 -0700 (PDT) X-Received: by 10.194.134.161 with SMTP id pl1mr12761619wjb.31.1370084863763; Sat, 01 Jun 2013 04:07:43 -0700 (PDT) Received: from localhost ([2.26.203.247]) by mx.google.com with ESMTPSA id fz8sm9523433wib.2.2013.06.01.04.07.42 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 01 Jun 2013 04:07:43 -0700 (PDT) From: Richard Sandiford To: "Moore\, Catherine" Mail-Followup-To: "Moore\, Catherine" ,"binutils\@sourceware.org" , rdsandiford@googlemail.com Cc: "binutils\@sourceware.org" Subject: Re: [Patch] Gas support for MIPS Compact EH References: Date: Sat, 01 Jun 2013 11:07:00 -0000 In-Reply-To: (Catherine Moore's message of "Fri, 31 May 2013 18:56:15 +0000") Message-ID: <87k3me9jia.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2013-06/txt/msg00004.txt.bz2 "Moore, Catherine" writes: > Index: doc/as.texinfo > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > RCS file: /cvs/src/src/gas/doc/as.texinfo,v > retrieving revision 1.266 > diff -p -u -r1.266 as.texinfo > --- doc/as.texinfo 29 Apr 2013 13:38:58 -0000 1.266 > +++ doc/as.texinfo 31 May 2013 17:24:20 -0000 > @@ -4479,6 +4479,9 @@ if @var{section_list} is @code{.debug_fr > To emit both use @code{.eh_frame, .debug_frame}. The default if this > directive is not used is @code{.cfi_sections .eh_frame}. >=20=20 > +On targets that support compact unwinding tables these can be generated > +by specifying @code{.eh_frame_header} instead of @code{.eh_frame}. Should this be .eh_frame_entry instead of .eh_frame_header? > @@ -4514,6 +4517,23 @@ argument is not present, otherwise secon > or a symbol name. The default after @code{.cfi_startproc} is @code{.cfi= _lsda 0xff}, > no LSDA. >=20=20 > +@section @code{.cfi_inline_lsda} [@var{align}] > +@code{.cfi_inline_lsda} marks the start of a LSDA data section and > +switches to the corresponding @code{.gnu.extab} section. > +It must be preceded by a CFI block containing a @code{.cfi_lsda} directi= ve and > +is only valid when generating compact EH frames (i.e. > +with @code{.cfi_sections eh_frame_entry}. > + > +If a compact encoding is being used then the table header and unwinding > +opcodes will be generated at this point, so that they are immediately > +followed by the LSDA data. The symbol referenced by the @code{.cfi_lsda} > +directive should still be defined in case a fallback FDE based encoding > +is used. > + > +The optional @var{align} argument specifies the alignment required. > +The alignment is specified as a power of two, as with the > +@code{.p2align} directive. Hmm, switching sections and emitting data feels very different in style from the other .cfi directives, which just annotate code without changing the flow of assembly. I'd like to know other people's thoughts on this. Also, how do you terminate the LSDA? The documentation doesn't say (but should :-)) and I couldn't see this directive in the spec either. "If a compact encoding is being used" seems redundant, since it comes just after the bit saying "only valid when generating compact EH frames". There need to be tests for all of this. TBH, without tests, and without an explanation of what the code is doing, I found this patch pretty hard to review. E.g.: > @@ -129,7 +140,12 @@ get_debugseg_name (segT seg, const char=20 > dot =3D strchr (name + 1, '.'); >=20=20 > if (!dollar && !dot) > - name =3D ""; > + { > + if (compact_eh && strcmp (name, ".text") !=3D 0) > + return concat (base_name, ".", name, NULL); > + > + name =3D ""; > + } why is this change needed? I.e., for a text section called something like .foobar, why does compact_eh need to put things in a section name ending in "..foobar", rather than in the main EH section? I assume the double dots are deliberate, e.g. to avoid confusion with ".text.foobar"? > @@ -161,6 +177,9 @@ alloc_debugseg_item (segT seg, int subse > static segT > is_now_linkonce_segment (void) > { > + if (compact_eh) > + return now_seg; > + > if ((bfd_get_section_flags (stdoutput, now_seg) > & (SEC_LINK_ONCE | SEC_LINK_DUPLICATES_DISCARD > | SEC_LINK_DUPLICATES_ONE_ONLY | SEC_LINK_DUPLICATES_SAME_SIZE Why is this change needed? > @@ -180,7 +199,11 @@ make_debug_seg (segT cseg, char *name, i > segT r; > flagword flags; >=20=20 > +#ifdef tc_make_debug_seg > + r =3D tc_make_debug_seg (cseg, name); > +#else > r =3D subseg_new (name, 0); > +#endif Why is this change needed? And what does the new hook do? It should be documented in internals.texi. Do we really want to change the behaviour for traditional DWARF EH too? > @@ -833,14 +859,15 @@ dot_cfi_personality (int ignored ATTRIBU > } >=20=20 > if ((encoding & 0xff) !=3D encoding > - || ((encoding & 0x70) !=3D 0 > + || ((((encoding & 0x70) !=3D 0 > #if CFI_DIFF_EXPR_OK || defined tc_cfi_emit_pcrel_expr > - && (encoding & 0x70) !=3D DW_EH_PE_pcrel > + && (encoding & 0x70) !=3D DW_EH_PE_pcrel > #endif > ) > /* leb128 can be handled, but does something actually need it? */ > - || (encoding & 7) =3D=3D DW_EH_PE_uleb128 > - || (encoding & 7) > DW_EH_PE_udata8) > + || (encoding & 7) =3D=3D DW_EH_PE_uleb128 > + || (encoding & 7) > DW_EH_PE_udata8) > + && !tc_cfi_special_encoding (encoding))) > { > as_bad (_("invalid or unsupported encoding in .cfi_personality")); > ignore_rest_of_line (); What does a "special" encoding mean? Again, this hook should be documented in internals.texi. And do we really want to change the set of encodings that are allowed for DWARF, even on MIPS systems that predate compat EH? > @@ -1042,6 +1072,13 @@ dot_cfi_sections (int ignored ATTRIBUTE_ > sections |=3D CFI_EMIT_eh_frame; > else if (strncmp (name, ".debug_frame", sizeof ".debug_frame") =3D=3D 0) > sections |=3D CFI_EMIT_debug_frame; > +#if SUPPORT_COMPACT_EH > + else if (strncmp (name, ".eh_frame_entry", sizeof ".eh_frame_entry") = =3D=3D 0) > + { > + compact_eh =3D TRUE; > + sections |=3D CFI_EMIT_eh_frame | CFI_EMIT_target; > + } > +#endif > #ifdef tc_cfi_section_name > else if (strcmp (name, tc_cfi_section_name) =3D=3D 0) > sections |=3D CFI_EMIT_target; I think there needs to be more error checking here. Do we support mixtures of DWARF and new-format EH info in the same .s file? If not, we should check for it, including making sure that later .cfi_sections don't contradict earlier ones. (At the moment, the last one wins, except for the CFI_EMIT_target handling.) If we do support mixtures, shouldn't something clear compat_eh, since compat_eh is checked in parsing contexts? Why is the CFI_EMIT_target needed? > @@ -1125,16 +1162,147 @@ dot_cfi_endproc (int ignored ATTRIBUTE_U > return; > } >=20=20 > - fde =3D frchain_now->frch_cfi_data->cur_fde_data; > + last_fde =3D frchain_now->frch_cfi_data->cur_fde_data; >=20=20 > cfi_end_fde (symbol_temp_new_now ()); >=20=20 > demand_empty_rest_of_line (); >=20=20 > - if ((cfi_sections & CFI_EMIT_target) !=3D 0) > - tc_cfi_endproc (fde); > + if ((cfi_sections & CFI_EMIT_target) !=3D 0 > + || (compact_eh && (cfi_sections & CFI_EMIT_eh_frame) !=3D 0)) > + tc_cfi_endproc (last_fde); > } I think it'd be better to use a separate hook, especially if the idea is that other targets could use compact EH info. Targets might then need a "legacy" tc_cfi_endproc as well as a "new" hook. > +#if SUPPORT_COMPACT_EH > +static void > +output_compact_unwind_data (struct fde_entry *fde, int align) > +{ > + int data_size =3D fde->eh_data_size + 2; > + int amask; > + char *p; > + char *end; > + > + fde->eh_loc =3D symbol_temp_new_now (); > + if (fde->per_encoding !=3D DW_EH_PE_omit) > + data_size +=3D 4; This 4 shouldn't be hardcoded. Hopefully an unconditional: data_size +=3D encoding_size (fde->per_encoding); would work; if not, please extend encoding_size. We then have: > + if (fde->per_encoding !=3D DW_EH_PE_omit) > + { > + *(p++) =3D 0; > + md_number_to_chars (p, 0, 4); > + tc_cfi_fix_eh_ref (p, &fde->personality); > + p +=3D 4; where tc_cfi_fix_eh_ref emits an R_MIPS_EH relocation regardless of which personality encoding is used. AIUI, the encoding must be one of the "special" ones allowed by tc_cfi_special_encoding, so we should check for that. The tc_cfi_fix_eh_ref and tc_cfi_emit_expr hooks don't seem very consistent; the former relies on the caller to clear the bytes, whereas the latter is supposed to do it itself. Neither of the hooks really seem to be doing anything target-specific except for the all-important job of picking a reloc. I think it would be cleaner to have a hook that returns the reloc instead. In the case of tc_cfi_emit_expr, this could be done by making tc_cfi_special_encoding return the reloc for an encoding, or BFD_RELOC_NONE for unsupported encodings. Also, this is more of a design point, but I find the handling of the personality encoding and R_MIPS_EH handling a bit confusing. To quote: --------------------------------------------------------------------- 11 Relocations A new static relocation, R_MIPS_EH, is defined. The semantics of this relocation depend on whether static or dynamic linking is provided. A GNU extension using relocation number 249 shall be used. The relocation address need not be naturally aligned. 11.1 Static code For static code generation, the calculation is the same as an R_MIPS_REL32 relocation. At runtime, the following expression provides the relocated value, if 'ptr' points to the relocation location. =E2=80=A2 (ptrdiff_t)ptr + *(int32_t __attribute__((packed))*)ptr [...] 11.2 PIC code For PIC code generation, a 32- or 64-bit GOT-table entry must be allocated to refer to the (dynamically resolved) target address. Once the GOT entry has been allocated, the static calculation is as for an R_MIPS_GPREL32 relocation (except that the symbol is externally visible). The GOT- slot has an associated R_MIPS_{32,64} dynamic relocation emitted =E2=80=93 and that will of course be at a naturally alig= ned location At runtime, the following expression provides the relocated value, if 'ptr' points to the relocation location, and 'gp' is the global pointer value: =E2=80=A2 *(ptrdiff_t *)((ptrdiff_t)gp + *(int32_t __attribute__((packed)) = *)ptr) [...] --------------------------------------------------------------------- So as I understand the first sentence, it's actually the linker that decides whether R_MIPS_EH relocations act as direct PC-relative references (11.1) or as indirect datarel references (11.2). It's therefore the linker that decides what DWARF encoding R_MIPS_EH fields use. The linker then records that choice in the .eh_frame_hdr. Is that right? If so, the assembler seems to be associating R_MIPS_EH with specific DWARF encodings, even though the interpretation of R_MIPS_EH isn't known at that stage. I.e. the code reads: #define tc_cfi_special_encoding(e) \ ((e) =3D=3D (DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect) \ || (e) =3D=3D (DW_EH_PE_sdata4 | DW_EH_PE_pcrel)) void mips_cfi_emit_expr (expressionS *exp, int encoding) { char *p; p =3D frag_more(4); md_number_to_chars (p, 0, 4); if ((encoding & 0x70) =3D=3D DW_EH_PE_datarel) mips_cfi_fix_eh_ref (p, exp); else { fix_new (frag_now, p - frag_now->fr_literal, 4, exp->X_add_symbol, exp->X_add_number, TRUE, BFD_RELOC_32_PCREL); } } where mips_cfi_fix_eh_ref emits an R_MIPS_EH. So the code appears to be using R_MIPS_EH for the DWARF encoding: DW_EH_PE_sdata4 | DW_EH_PE_datarel | DW_EH_PE_indirect even though there's no guarantee that the final value will be either datarel or indirect. Or, to put it another way, why are we making the choice between R_MIPS_PC32 and R_MIPS_EH at assembly time in mips_cfi_emit_expr, but not in mips_cfi_fix_eh_ref, even though the patch appears to allow the same two encodings in both casees? > + memcpy (p, fde->eh_data, fde->eh_data_size); > + p +=3D fde->eh_data_size; > + while (p !=3D end) > + *(p++) =3D 0x5f; > + > + *(p++) =3D 0x5c; Please use something more mnemonic than 0x5f and 0x5c. Are these the "Spare" and "PC =3D VR[31]; Finish" encodings from page 16? If so, they seem target-specific, so should probably use tc_* #defines. > +static void > +dot_cfi_inline_lsda (int ignored ATTRIBUTE_UNUSED) > +{ > + segT cfi_seg, ccseg; > + int align; > + long max_alignment =3D 28; > + > + if (!last_fde) > + { > + as_bad (_("unexpected .cfi_inline_lsda")); > + ignore_rest_of_line (); > + return; > + } > + > + if (!compact_eh > + || (last_fde->sections & CFI_EMIT_eh_frame) =3D=3D 0 > + || (last_fde->eh_header_type !=3D EH_COMPACT_LEGACY > + && last_fde->eh_header_type !=3D EH_COMPACT_HAS_LSDA)) > + > + { > + as_bad (_(".cfi_inline_lsda not valid for this frame")); > + ignore_rest_of_line (); > + return; > + } Excess blank line. The first two lines of the if are clear enough, but what is the eh_header_type condition doing? It looks like one of the cases it catches is where two LSDAs are specified for the same frame. If so, I think a better error message should be given in that case. > + demand_empty_rest_of_line (); > + ccseg =3D CUR_SEG (last_fde); > + /* Open .gnu_extab section. */ > + cfi_seg =3D get_cfi_seg (ccseg, ".gnu_extab", > + (SEC_ALLOC | SEC_LOAD | SEC_DATA > + | DWARF2_EH_FRAME_READ_ONLY), > + 1); > +#ifdef md_fix_up_eh_frame > + md_fix_up_eh_frame (cfi_seg); > +#else > + (void) cfi_seg; > +#endif > + > + frag_align (align, 0, 0); > + record_alignment (now_seg, align); > + if (last_fde->eh_header_type =3D=3D EH_COMPACT_HAS_LSDA) > + output_compact_unwind_data (last_fde, align); Please could you explain the EH_COMPACT_LEGACY handling here? > @@ -1479,7 +1647,11 @@ output_cie (struct cie_entry *cie, bfd_b > offsetT size =3D encoding_size (cie->per_encoding); > out_one (cie->per_encoding); > exp =3D cie->personality; > - if ((cie->per_encoding & 0x70) =3D=3D DW_EH_PE_pcrel) > + if (tc_cfi_special_encoding (cie->per_encoding)) > + { > + tc_cfi_emit_expr (&exp, cie->per_encoding); > + } Excess braces. > @@ -1617,7 +1804,11 @@ output_fde (struct fde_entry *fde, struc > if (fde->lsda_encoding !=3D DW_EH_PE_omit) > { > exp =3D fde->lsda; > - if ((fde->lsda_encoding & 0x70) =3D=3D DW_EH_PE_pcrel) > + if (tc_cfi_special_encoding (cie->lsda_encoding)) > + { > + tc_cfi_emit_expr (&exp, cie->lsda_encoding); > + } Same here. > +#if SUPPORT_COMPACT_EH > +static void > +cfi_emit_eh_header (symbolS *sym, bfd_vma addend) > { > - if (SUPPORT_FRAME_LINKONCE) > + expressionS exp; > + > + exp.X_add_number =3D addend; > + exp.X_add_symbol =3D sym; > + if (tc_cfi_special_encoding (DW_EH_PE_sdata4 | DW_EH_PE_pcrel)) > { > - struct dwcfi_seg_list *l; > + tc_cfi_emit_expr (&exp, DW_EH_PE_sdata4 | DW_EH_PE_pcrel); > + } Here too. > + /* Open .eh_frame section. */ > + cfi_seg =3D get_cfi_seg (ccseg, ".gnu_extab", > + (SEC_ALLOC | SEC_LOAD | SEC_DATA > + | DWARF2_EH_FRAME_READ_ONLY), > + 1); Pasto, this isn't .eh_frame. > @@ -15712,7 +15712,8 @@ md_apply_fix (fixS *fixP, valueT *valP,=20 > || fixP->fx_r_type =3D=3D BFD_RELOC_MICROMIPS_SUB > || fixP->fx_r_type =3D=3D BFD_RELOC_VTABLE_INHERIT > || fixP->fx_r_type =3D=3D BFD_RELOC_VTABLE_ENTRY > - || fixP->fx_r_type =3D=3D BFD_RELOC_MIPS_TLS_DTPREL64); > + || fixP->fx_r_type =3D=3D BFD_RELOC_MIPS_TLS_DTPREL64 > + || fixP->fx_r_type =3D=3D BFD_RELOC_NONE); >=20=20 > buf =3D fixP->fx_frag->fr_literal + fixP->fx_where; >=20=20 > @@ -15951,6 +15952,7 @@ md_apply_fix (fixS *fixP, valueT *valP,=20 > S_SET_WEAK (fixP->fx_addsy); > break; >=20=20 > + case BFD_RELOC_NONE: > case BFD_RELOC_VTABLE_ENTRY: > fixP->fx_done =3D 0; > break; Why are these two hunks needed? > @@ -19793,3 +19795,289 @@ tc_mips_regname_to_dw2regnum (char *regn >=20=20 > return regnum; > } > + > +#if defined (OBJ_ELF) > +segT > +mips_make_debug_seg (segT cseg, char *name) > +{ > + const char *group_name =3D NULL; > + int linkonce; > + int flags =3D 0; > + > + if (cseg && (cseg->flags & SEC_LINK_ONCE) !=3D 0) > + { > + group_name =3D elf_group_name (cseg); > + if (group_name =3D=3D NULL) > + { > + as_bad (_("Group section `%s' has no group signature"), > + segment_name (cseg)); > + } > + flags |=3D SHF_GROUP; > + linkonce =3D 1; > + } > + obj_elf_change_section (name, SHT_PROGBITS, flags, 0, group_name, link= once, 0); > + return now_seg; > +} > + This function doesn't seem to contain any MIPS-specific code. If a hook really is needed, then I think this implementation belongs in obj-elf.c rather than tc-mips.c. > +/* Attempt to generate compact frame unwind encodings. */ > + > +void > +mips_cfi_endproc (struct fde_entry *fde ATTRIBUTE_UNUSED) > +{ > + struct cfi_insn_data *insn; > + int reg; > + offsetT cfa_offset =3D 0; > + offsetT next_offset =3D 0; > + bfd_boolean reg_offset[MIPS_NUM_UNWIND_REGS]; > + int cfa_reg =3D 29; > + bfd_byte opbuff[40]; Why 40? As a maths exam might say, please show your working :-) > + /* Frame pointer must be callee caved. */ > + if (cfa_reg < 16 > + || (cfa_reg > 23 && cfa_reg < 29) > + || cfa_reg > 30) > + return; > + > + /* Can only increment stack pointer. */ > + if (cfa_offset < 0) > + return; > + > + /* Stack must be aligned. */ > + if ((cfa_offset & (stack_align - 1)) !=3D 0) > + return; > + > + /* Registers must be contiguous. There's only limited benefit in > + supprting more complex layouts, so For now we also require they > + be in order. */ > + next_offset =3D -gpr_size; > + for (i =3D MIPS_NUM_UNWIND_REGS - 1; i >=3D 0; i--) > + { > + if (reg_offset[i] =3D=3D 0) > + continue; > + > + if (reg_offset[i] !=3D next_offset) > + return; > + > + next_offset -=3D gpr_size; > + } What happens after these early returns? Are they user errors (in which case we should report them), or do we fall back to something else (in which case a boolean return value would probably make sense)? > + if (fde->lsda_encoding !=3D DW_EH_PE_omit) > + fde->eh_header_type =3D EH_COMPACT_HAS_LSDA; > + else if (num_ops <=3D 3 && fde->per_encoding =3D=3D DW_EH_PE_omit) > + fde->eh_header_type =3D EH_COMPACT_INLINE; > + else > + fde->eh_header_type =3D EH_COMPACT_OUTLINE; Why does this part need to be in MIPS-specific code? Thanks, Richard