From patchwork Thu Dec 4 15:30:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Maciej W. Rozycki" X-Patchwork-Id: 4066 Received: (qmail 8898 invoked by alias); 4 Dec 2014 15:31:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 8884 invoked by uid 89); 4 Dec 2014 15:31:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL, BAYES_00, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 04 Dec 2014 15:31:08 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-02.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1XwYMy-0000kS-EP from Maciej_Rozycki@mentor.com ; Thu, 04 Dec 2014 07:31:04 -0800 Received: from localhost (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server (TLS) id 14.3.181.6; Thu, 4 Dec 2014 15:31:02 +0000 Date: Thu, 4 Dec 2014 15:30:57 +0000 From: "Maciej W. Rozycki" To: Doug Evans CC: Joel Brobecker , , Rich Fuhler , Richard Sandiford Subject: Re: [PATCH v2 1/2] ISA bit treatment on the MIPS platform In-Reply-To: Message-ID: References: <20120611182043.GA7597@adacore.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 On Mon, 17 Nov 2014, Doug Evans wrote: > "Maciej W. Rozycki" writes: > > [...] > > 2014-10-06 Maciej W. Rozycki > > Maciej W. Rozycki > > Pedro Alves > > > > * gdbarch.sh (elf_make_msymbol_special): Change type to `F', > > remove `predefault' and `invalid_p' initializers. > > (make_symbol_special): New architecture method. > > (adjust_dwarf2_addr, adjust_dwarf2_line): Likewise. > > (objfile, symbol): New declarations. > > * arch-utils.h (default_elf_make_msymbol_special): Remove > > prototype. > > (default_make_symbol_special): New prototype. > > (default_adjust_dwarf2_addr): Likewise. > > (default_adjust_dwarf2_line): Likewise. > > * mips-tdep.h (mips_unmake_compact_addr): New prototype. > > * arch-utils.c (default_elf_make_msymbol_special): Remove > > function. > > (default_make_symbol_special): New function. > > (default_adjust_dwarf2_addr): Likewise. > > (default_adjust_dwarf2_line): Likewise. > > * dwarf2-frame.c (decode_frame_entry_1): Call > > `gdbarch_adjust_dwarf2_addr'. > > * dwarf2loc.c (dwarf2_find_location_expression): Likewise. > > * dwarf2read.c (create_addrmap_from_index): Likewise. > > (process_psymtab_comp_unit_reader): Likewise. > > (add_partial_symbol): Likewise. > > (add_partial_subprogram): Likewise. > > (process_full_comp_unit): Likewise. > > (read_file_scope): Likewise. > > (read_func_scope): Likewise. Call `gdbarch_make_symbol_special'. > > (read_lexical_block_scope): Call `gdbarch_adjust_dwarf2_addr'. > > (read_call_site_scope): Likewise. > > (dwarf2_ranges_read): Likewise. > > (dwarf2_record_block_ranges): Likewise. > > (read_attribute_value): Likewise. > > (dwarf_decode_lines_1): Call `gdbarch_adjust_dwarf2_line'. > > (new_symbol_full): Call `gdbarch_adjust_dwarf2_addr'. > > * elfread.c (elf_symtab_read): Don't call > > `gdbarch_elf_make_msymbol_special' if unset. > > * mips-linux-tdep.c (micromips_linux_sigframe_validate): Strip > > the ISA bit from the PC. > > * mips-tdep.c (mips_unmake_compact_addr): New function. > > (mips_elf_make_msymbol_special): Set the ISA bit in the symbol's > > address appropriately. > > (mips_make_symbol_special): New function. > > (mips_pc_is_mips): Set the ISA bit before symbol lookup. > > (mips_pc_is_mips16): Likewise. > > (mips_pc_is_micromips): Likewise. > > (mips_pc_isa): Likewise. > > (mips_adjust_dwarf2_addr): New function. > > (mips_adjust_dwarf2_line): Likewise. > > (mips_read_pc, mips_unwind_pc): Keep the ISA bit. > > (mips_addr_bits_remove): Likewise. > > (mips_skip_trampoline_code): Likewise. > > (mips_write_pc): Don't set the ISA bit. > > (mips_eabi_push_dummy_call): Likewise. > > (mips_o64_push_dummy_call): Likewise. > > (mips_gdbarch_init): Install `mips_make_symbol_special', > > `mips_adjust_dwarf2_addr' and `mips_adjust_dwarf2_line' gdbarch > > handlers. > > * solib.c (gdb_bfd_lookup_symbol_from_symtab): Get > > target-specific symbol address adjustments. > > * gdbarch.h: Regenerate. > > * gdbarch.c: Regenerate. > > > > 2014-10-06 Maciej W. Rozycki > > > > gdb/testsuite/ > > * gdb.base/func-ptrs.c: New file. > > * gdb.base/func-ptrs.exp: New file. > > Ummm, bleah. Yeah, that's a consequence of a bad decision made (or the lack of one and the resulting semantics that spontaneously happened) many years ago as MIPS16 support was implemented. References to compressed code are made in an inconsistent way across DWARF records with information lost from some of them. That information has to be recovered from the symbol table. The missing information could be stored separately in DWARF records I suppose, but that won't help with many binaries out there; MIPS16 support has been there for well over a decade -- our first record is from Jan 1997 and support in GCC/GAS must have been there since around that time too. > Going forward dwarf2*.c are going to be measurably more of a pain > to maintain than they already are. > I notice a fair bit of mips code gets a lot simpler. And some stuff actually starts working, that never did before. I posted the full story along the original patch submission: http://sourceware.org/ml/gdb-patches/2012-05/msg00515.html > Such is life I guess. I tried hard figuring out a way that would avoid making changes to generic DWARF code and could not find one. Once the DWARF records have been processed there doesn't appear to me to be a way for places across GDB's generic code to gather the pieces of information required without turning upside down those places for a change. > Just a few comments. > > >[...] > > Index: gdb-fsf-trunk-quilt/gdb/gdbarch.sh > > =================================================================== > > --- gdb-fsf-trunk-quilt.orig/gdb/gdbarch.sh 2014-10-03 13:52:46.000000000 +0100 > > +++ gdb-fsf-trunk-quilt/gdb/gdbarch.sh 2014-10-03 14:50:26.000000000 +0100 > > @@ -635,8 +635,11 @@ m:int:in_solib_return_trampoline:CORE_AD > > # which don't suffer from that problem could just let this functionality > > # untouched. > > m:int:in_function_epilogue_p:CORE_ADDR addr:addr:0:generic_in_function_epilogue_p::0 > > -f:void:elf_make_msymbol_special:asymbol *sym, struct minimal_symbol *msym:sym, msym::default_elf_make_msymbol_special::0 > > +F:void:elf_make_msymbol_special:asymbol *sym, struct minimal_symbol *msym:sym, msym > > f:void:coff_make_msymbol_special:int val, struct minimal_symbol *msym:val, msym::default_coff_make_msymbol_special::0 > > +f:void:make_symbol_special:struct symbol *sym, struct objfile *objfile:sym, objfile::default_make_symbol_special::0 > > +f:CORE_ADDR:adjust_dwarf2_addr:CORE_ADDR pc:pc::default_adjust_dwarf2_addr::0 > > +f:CORE_ADDR:adjust_dwarf2_line:CORE_ADDR addr, int rel:addr, rel::default_adjust_dwarf2_line::0 > > I don't know where the best place to put this is, but the adjust_dwarf2 > functions need a lot more documentation. Why do they exist? > What problem do they solve? Something along those lines. > References to existing documentation is fine, as long as they're sufficient. The source code has so far been the documentation I am afraid. Which I think also contributes to the mess. Implied bits are buried in GCC in gcc/config/mips/mips.h and libgcc/unwind-dw2.c for example, see the comment around `MASK_RETURN_ADDR' in the former and the comment at the beginning of `execute_cfa_program' in the latter. The assumptions made in these both places make issues nasty, adding 1 to the return address in the latter place is incompatible with how the ISA bit is interpreted everywhere else. This function post-dates the introduction of the MIPS16 instruction set I believe and therefore I think it should have been designed with the issues around it in mind, e.g. I used the patch below in the early stage of investigation of possible solutions for the problem addressed here. > I realize I can find that out with some manual labor, e.g., finding > which arches provide an implementation of these functions, and reading them, > but even when I get to mips-tdep.c I'm left wondering *why* > the mips port needs this (e.g., why can't the problem be solved > differently?). I'm more familiar with arm/thumb, and of course > the first thing that comes to mind is that arm/thumb doesn't need this > (maybe it does, but we've gotten this far without it). I suspect that it does, but it can be found out easily as this is why I included gdb.base/func-ptrs.exp with the change. Any target that fails the test cases it contains will need addressing the problem, possibly just by implementing the equivalent of the MIPS bits posted here. > It would help > to educate the reader of this code as to why mips is different. > Maybe this patch is the best solution. If so, and I'm going to assume that > it is, then let's get this documented in the code. > Even just a reference to existing docs is fine. > mips-tdep.c is a big file though - I don't want to have to read all > of it just to understand why adjust_dwarf2_line exists. I'll try summarising the key points for the new hooks in gdbarch.sh (that is propagated by the script to gdbarch.h) and for the MIPS implementation in mips-tdep.c. I'll post the final version for you (and Joel) to comment on before I push it, in the next few days. > > Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c > > =================================================================== > > --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2014-10-03 13:52:46.000000000 +0100 > > +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2014-10-06 00:11:55.028611249 +0100 > >[...] > > @@ -391,6 +406,29 @@ msymbol_is_micromips (struct minimal_sym > > return MSYMBOL_TARGET_FLAG_2 (msym); > > } > > > > +/* Set the ISA bit in the main symbol too, complementing the corresponding > > + minimal symbol setting and reflecting the run-time value of the symbol. */ > > + > > +static void > > +mips_make_symbol_special (struct symbol *sym, struct objfile *objfile) > > +{ > > + if (SYMBOL_CLASS (sym) == LOC_BLOCK) > > + { > > + CORE_ADDR compact_block_start; > > + struct bound_minimal_symbol msym; > > + > > + compact_block_start = BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) | 1; > > + msym = lookup_minimal_symbol_by_pc (compact_block_start); > > + if (msym.minsym && !msymbol_is_mips (msym.minsym)) > > + { > > + /* We are in symbol reading so it is OK to cast away constness. */ > > + struct block *block = (struct block *) SYMBOL_BLOCK_VALUE (sym); > > + > > + BLOCK_START (block) = compact_block_start; > > + } > > + } > > +} > > This function would be easier to read if the assignment to block > was moved up and its value used in both invocations of BLOCK_START. Fixed. It looks to me like an outcome from the long history of this function (dating back to Jan 2008) and the number of adjustments it gathered as the structures it uses evolved. In the process I missed the possibility for this simplification. > >[...] > > +/* Recalculate the line record requested so that the resulting PC has the > > + ISA bit set correctly, used by DWARF-2 machinery. */ > > + > > +static CORE_ADDR > > +mips_adjust_dwarf2_line (CORE_ADDR addr, int rel) > > +{ > > + static CORE_ADDR adj_pc; > > + static CORE_ADDR pc; > > + CORE_ADDR isa_pc; > > + > > + pc = rel ? pc + addr : addr; > > + isa_pc = mips_adjust_dwarf2_addr (pc); > > + addr = rel ? isa_pc - adj_pc : isa_pc; > > + adj_pc = isa_pc; > > + return addr; > > +} > > If this function was a "method", we could remove the "static" vars, > but this is ok I guess. > There's magic going on here, carrying the value of pc,adj_pc > from one call to the next. There's a rule here that this will > be called with rel == 0, before any calls with rel != 0. > It's kinda obvious now, but it would have helped this reader to see > it written down. Indeed, already noticed by Joel. This piece gave me a bit of a headache and after considering available alternatives I decided to put the least burden on our DWARF machinery and contain all the stuff within the MIPS backend. This way people outside the MIPS world do not have to be bothered with it, and I think code is also relatively simple, which is going to help with long-term maintenance. The only drawback is backend's state has to be initialised and consequently an initial call with `rel == 0' is required. I'll document it at the call site and here as well. And here's the experimental patch for GCC EH unwinder I referred to above, published for posterity. Maciej gcc-mips-unwind-keep-isa-bit.diff Index: gcc-fsf-trunk-quilt/gcc/config/mips/mips.h =================================================================== --- gcc-fsf-trunk-quilt.orig/gcc/config/mips/mips.h 2012-05-01 20:44:04.000000000 +0100 +++ gcc-fsf-trunk-quilt/gcc/config/mips/mips.h 2012-05-02 23:44:40.375576966 +0100 @@ -2049,6 +2049,7 @@ enum reg_class #define RETURN_ADDR_RTX mips_return_addr +#if 0 /* Mask off the MIPS16 ISA bit in unwind addresses. The reason for this is a little subtle. When unwinding a call, @@ -2076,6 +2077,9 @@ enum reg_class Masking off the ISA bit means that the target-independent code will search for "(RA & -2) - 1", which is guaranteed to be odd. */ #define MASK_RETURN_ADDR GEN_INT (-2) +#else +#define MIN_INSN_SIZE 2 +#endif /* Similarly, don't use the least-significant bit to tell pointers to Index: gcc-fsf-trunk-quilt/libgcc/unwind-dw2.c =================================================================== --- gcc-fsf-trunk-quilt.orig/libgcc/unwind-dw2.c 2012-05-01 20:41:24.275429140 +0100 +++ gcc-fsf-trunk-quilt/libgcc/unwind-dw2.c 2012-05-02 23:44:40.385574339 +0100 @@ -927,7 +927,8 @@ execute_cfa_program (const unsigned char In signal frames, return address is after last completed instruction, so we add 1 to return address to make the comparison <=. */ while (insn_ptr < insn_end - && fs->pc < context->ra + _Unwind_IsSignalFrame (context)) + && fs->pc <= context->ra - (_Unwind_IsSignalFrame (context) + ? 0 : MIN_INSN_SIZE)) { unsigned char insn = *insn_ptr++; _uleb128_t reg, utmp; @@ -1177,7 +1178,8 @@ uw_frame_state_for (struct _Unwind_Conte if (context->ra == 0) return _URC_END_OF_STACK; - fde = _Unwind_Find_FDE (context->ra + _Unwind_IsSignalFrame (context) - 1, + fde = _Unwind_Find_FDE (context->ra - (_Unwind_IsSignalFrame (context) + ? 0 : MIN_INSN_SIZE), &context->bases); if (fde == NULL) {