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

Joel Brobecker
2014-11-16T10:37:30Z

> 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