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

[Doug Evans <xdje42 at gmail dot com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> [...]
> 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.

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.