Re: [RFD+PATCH] ISA bit treatment on the MIPS platform

Joel Brobecker
2012-06-11T18:21:12Z

> I propose therefore to accept the existing inconsistencies and deal
> with them entirely within GDB. I have figured out that the ISA bit
> lost in various places can still be recovered as long as we have
> symbol information -- that'll have the st_other attribute correctly
> set to one of standard MIPS/MIPS16/microMIPS.

That seems reasonable to me.

> 2012-05-14 Maciej W. Rozycki <macro@codesourcery.com>
> Maciej W. Rozycki <macro@mips.com>
> Pedro Alves <pedro@codesourcery.com>
>
> * gdbarch.sh (make_symbol_special): New architecture method.
> (adjust_dwarf2_addr, adjust_dwarf2_line): Likewise.
> (objfile, symbol): New declarations.
> * arch-utils.h (default_make_symbol_special): New prototype.
> (default_adjust_dwarf2_addr): Likewise.
> (default_adjust_dwarf2_line): Likewise.
> * arch-utils.c (default_make_symbol_special): New function.
> * dwarf2-frame.c (decode_frame_entry_1): Call
> gdbarch_adjust_dwarf2_addr.
> * dwarf2loc.c (dwarf2_find_location_expression): Likewise.
> * dwarf2read.c (read_func_scope): Call
> gdbarch_make_symbol_special.
> (dwarf2_ranges_read): Call gdbarch_adjust_dwarf2_addr.
> (read_attribute_value): Likewise.
> (dwarf_decode_lines_1): Call gdbarch_adjust_dwarf2_line.
> * mips-tdep.c (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.

From the ChangeLog entry, it seems like Pedro was involved in the making
of that patch, so perhaps he could be a good reviewer? There are also
changes in dwarf2*, so perhaps people such as Doug and Tom could also
be involved....

Just some minor comments below:

> gdb-mips16-isa-bit.diff
> Index: gdb-fsf-trunk-quilt/gdb/mips-tdep.c
> ===================================================================
> --- gdb-fsf-trunk-quilt.orig/gdb/mips-tdep.c 2012-05-14 16:00:33.000000000 +0100
> +++ gdb-fsf-trunk-quilt/gdb/mips-tdep.c 2012-05-14 16:02:02.235560558 +0100
> @@ -358,9 +358,15 @@ mips_elf_make_msymbol_special (asymbol *
> return;
>
> if (ELF_ST_IS_MICROMIPS (elfsym->internal_elf_sym.st_other))
> - MSYMBOL_TARGET_FLAG_2 (msym) = 1;
> + {
> + MSYMBOL_TARGET_FLAG_2 (msym) = 1;
> + SYMBOL_VALUE_ADDRESS (msym) |= 1;
> + }
> else if (ELF_ST_IS_MIPS16 (elfsym->internal_elf_sym.st_other))
> - MSYMBOL_TARGET_FLAG_1 (msym) = 1;
> + {
> + MSYMBOL_TARGET_FLAG_1 (msym) = 1;
> + SYMBOL_VALUE_ADDRESS (msym) |= 1;
> + }

This remark is not to be considered as part of this patch's review,
since this is already an established practice, but I think we could do
better than using magic numbers for those flags. I understand they have
to be that way in the common code, but perhaps mips-tdep could define
aliases? Something like MSYMBOL_MIPS_TARGET_FLAG_BLAH?

> Index: gdb-fsf-trunk-quilt/gdb/arch-utils.c
[...]
> +void
> +default_make_symbol_special (struct symbol *sym, struct objfile *objfile)

Can you put a comment at the start of all these new functions to say
that the documentation is in the header file? For instance, I think
we typically say:

/* See arch-utils.h. */

This is to help someone reviewing the code to know that there is
in fact documentation.

> +/* Do nothing version of make_symbol_special. */
> +
> +void default_make_symbol_special (struct symbol *sym, struct objfile *objfile);
> +
> +/* Do nothing version of adjust_dwarf2_addr. */
> +
> +CORE_ADDR default_adjust_dwarf2_addr (CORE_ADDR pc);
> +
> +/* Do nothing version of adjust_dwarf2_line. */
> +
> +CORE_ADDR default_adjust_dwarf2_line (CORE_ADDR addr, int rel);

I would definitely mention gdbarch in the function comment.
Prior art uses:

/* Default implementation of gdbarch_displaced_hw_singlestep. */

Should we remain consistent with that? It can be a mix of both,
saying what method it implements, and then what it actually does, Eg:

/* Default version of gdbarch_make_symbol_special (does nothing). */

> Index: gdb-fsf-trunk-quilt/gdb/solib.c
> ===================================================================
> --- gdb-fsf-trunk-quilt.orig/gdb/solib.c 2012-05-14 15:56:45.000000000 +0100
> +++ gdb-fsf-trunk-quilt/gdb/solib.c 2012-05-14 16:01:34.645558637 +0100
> @@ -1384,8 +1384,27 @@ gdb_bfd_lookup_symbol_from_symtab (bfd *
>
> if (match_sym (sym, data))
> {
> + symaddr = sym->value;
> +
> + /* macro/2012-04-20: 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)
> + {
> + struct minimal_symbol msym;
> +
> + memset (&msym, 0, sizeof (msym));
> + SYMBOL_VALUE_ADDRESS (&msym) = symaddr;
> + gdbarch_elf_make_msymbol_special (target_gdbarch,
> + sym, &msym);
> + symaddr = SYMBOL_VALUE_ADDRESS (&msym);
> + }

Is there a way we could avoid that block if
gdbarch_elf_make_msymbol_special points to the default implementation?
It just seems sad to be doing all this work of creating a temporary symbol
for every solib symbol on non-mips targets...

One possible way would have been to provide no default value for
gdbarch_elf_make_msymbol_special, but then it makes the calling
of that method a little trickier. That could be worked around by
changing the default value of gdbarch_elf_make_msymbol_special to NULL,
and then having an extra wrapper function that call the gdbarch
method if not NULL or else does nothing.

--
Joel