From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27231 invoked by alias); 16 Nov 2014 10:37:30 -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 27219 invoked by uid 89); 16 Nov 2014 10:37:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Sun, 16 Nov 2014 10:37:26 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 0B7EF116598; Sun, 16 Nov 2014 05:37:25 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id ORF4qscdrwBb; Sun, 16 Nov 2014 05:37:24 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 9B8F7116594; Sun, 16 Nov 2014 05:37:22 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id EE55040F79; Sun, 16 Nov 2014 14:37:21 +0400 (RET) Date: Sun, 16 Nov 2014 10:37:00 -0000 From: Joel Brobecker To: "Maciej W. Rozycki" Cc: gdb-patches@sourceware.org, Rich Fuhler , Richard Sandiford Subject: Re: [PATCH v2 1/2] ISA bit treatment on the MIPS platform Message-ID: <20141116103721.GG5774@adacore.com> References: <20120611182043.GA7597@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-11/txt/msg00376.txt.bz2 > 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. Pre-approved, with the on minor documentation request below. > 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 know we haven't been all that good in the past at documenting gdbarch methods, but new entries should be fully documented. That way, all arch-specific implementations can then document with a reference to the documentation in gdbarch.sh/gdbarch.h. Would you mind documenting the ones you're adding? This is not a requirement for your patch, but if you happen to be able to quickly document elf_make_msymbol_special as well, that would be a very welcome and appreciated change. > +/* 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; > +} I got to stare that his code for quite a while, because of the use of the private static variables, which means that the result of a call to that function depends on the previous call. I am guessing that this is the reason behind the following hunk earlier in the patch: - CORE_ADDR address = 0; + CORE_ADDR address = gdbarch_adjust_dwarf2_line (gdbarch, 0, 0); (to initialize the context). I think we all know that if makes the implementation a little harder to maintain in the long run, but after thinking over it for a while, I tend to like the fact that this helps making the chhanges to dwarf2read.c a little simpler, with negative impact being only on the MIPS target. So while I wish things were perfect, I'm have this feeling that the above is a "good deal" for core GDB ;-). > +++ gdb-fsf-trunk-quilt/gdb/solib.c 2014-10-03 14:50:26.398945943 +0100 > @@ -1444,8 +1444,28 @@ gdb_bfd_lookup_symbol_from_symtab (bfd * > > if (match_sym (sym, data)) > { > + struct gdbarch *gdbarch = target_gdbarch (); > + symaddr = sym->value; > + > + /* Some ELF targets fiddle with addresses of symbols they > + consider special. They use minimal symbols to do that > + and this is needed for correct breakpoint placement, > + but we do not have full data here to build a complete > + minimal symbol, so just set the address and let the > + targets cope with that. */ > + if (bfd_get_flavour (abfd) == bfd_target_elf_flavour > + && gdbarch_elf_make_msymbol_special_p (gdbarch)) > + { > + struct minimal_symbol msym; > + > + memset (&msym, 0, sizeof (msym)); > + SET_MSYMBOL_VALUE_ADDRESS (&msym, symaddr); > + gdbarch_elf_make_msymbol_special (gdbarch, sym, &msym); > + symaddr = MSYMBOL_VALUE_RAW_ADDRESS (&msym); > + } > + > /* BFD symbols are section relative. */ > - symaddr = sym->value + sym->section->vma; > + symaddr += sym->section->vma; > break; FTR: I think I had some reservations about this part of the code, and you improved it to only do the work if gdbarch_elf_make_msymbol_special_p, which is a nice improvement anyway. But looking at it more closely, I probably missed at the time the fact that the minimal_symbol has a local scope. (duh!) Thank you, -- Joel