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

[Joel Brobecker <brobecker@adacore.com>]

>  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