Re: [PATCH v2 1/2] ISA bit treatment on the MIPS platform

[Joel Brobecker <brobecker@adacore.com>]

> 2014-10-06  Maciej W. Rozycki  <macro@codesourcery.com>
>             Maciej W. Rozycki  <macro@mips.com>
>             Pedro Alves  <pedro@codesourcery.com>
> 
> 	* 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  <macro@codesourcery.com>
> 
> 	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