From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32217 invoked by alias); 17 Nov 2014 01:17:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 32207 invoked by uid 89); 17 Nov 2014 01:17:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL,BAYES_00,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-pa0-f42.google.com Received: from mail-pa0-f42.google.com (HELO mail-pa0-f42.google.com) (209.85.220.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 17 Nov 2014 01:17:38 +0000 Received: by mail-pa0-f42.google.com with SMTP id et14so6368336pad.1 for ; Sun, 16 Nov 2014 17:17:37 -0800 (PST) X-Received: by 10.66.231.37 with SMTP id td5mr25881463pac.80.1416187057027; Sun, 16 Nov 2014 17:17:37 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id v4sm7139890pbs.10.2014.11.16.17.17.35 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 16 Nov 2014 17:17:36 -0800 (PST) From: Doug Evans To: "Maciej W. Rozycki" Cc: Joel Brobecker , , Rich Fuhler , Richard Sandiford Subject: Re: [PATCH v2 1/2] ISA bit treatment on the MIPS platform References: <20120611182043.GA7597@adacore.com> Date: Mon, 17 Nov 2014 01:17:00 -0000 In-Reply-To: (Maciej W. Rozycki's message of "Mon, 6 Oct 2014 01:41:50 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00387.txt.bz2 "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. 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. Such is life I guess. 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. 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). 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. > v:int:cannot_step_breakpoint:::0:0::0 > v:int:have_nonsteppable_watchpoint:::0:0::0 > F:int:address_class_type_flags:int byte_size, int dwarf2_addr_class:byte_size, dwarf2_addr_class > @@ -1140,6 +1143,8 @@ struct target_ops; > struct obstack; > struct bp_target_info; > struct target_desc; > +struct objfile; > +struct symbol; > struct displaced_step_closure; > struct core_regset_section; > struct syscall; >[...] > 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. >[...] > +/* 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. Ok with those changes.