Re: [PATCH 01/24] MIPS: Handle run-time reconfigurable FPR size

["Maciej W. Rozycki" <macro@imgtec.com>]

Bhushan,

 Since this is virtually unchanged from the original submission I think it 
will be appropriate if you include a:

From: Maciej W. Rozycki <macro@codesourcery.com>

line at the beginning of the description to give a correct attribution.

>         * mips-irix-tdep.c (mips_irix_init_abi): Set
>         fp_register_size_fixed_p in gdbarch target data.

 This part is now gone along with IRIX support, so please remove it from 
the ChangeLog entry, and also make sure the rest of the ChangeLog entry is 
still consistent with the patch itself after adjustments.

> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index 8dc0566..6b9743b 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -1737,8 +1737,9 @@ mips_linux_init_abi (struct gdbarch_info info,
>  				    mips_gdb_signal_to_target);
>  

 You've lost the removal of the `tdesc_data' local variable from the 
original change here and this variable becomes unused -- was it by 
accident?

>    tdep->syscall_next_pc = mips_linux_syscall_next_pc;
> +  tdep->fp_register_size_fixed_p = 1;
>  
> -  if (tdesc_data)
> +  if (((struct gdbarch_tdep_info*)(info.tdep_info))->tdesc_data)

 Bad formatting here, you need a space after the cast.

>      {
>        const struct tdesc_feature *feature;
>  
> @@ -1753,8 +1754,8 @@ mips_linux_init_abi (struct gdbarch_info info,
>        feature = tdesc_find_feature (info.target_desc,
>  				    "org.gnu.gdb.mips.linux");
>        if (feature != NULL)
> -	tdesc_numbered_register (feature, tdesc_data, MIPS_RESTART_REGNUM,
> -				 "restart");
> +	tdesc_numbered_register (feature, (((struct gdbarch_tdep_info*)(info.tdep_info))->tdesc_data),

 Likewise.  Also lines are supposed to fit in 74 columns unless there is a 
good justification to make an exception, in which case they must not 
exceed 80 columns, as per GDB formatting style (please see 
<https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards> for 
details).  So this line needs to be wrapped.

 However, I think this will best be sorted out with an auxiliary local 
`tdep_info' variable, in which case you won't need a cast at all.

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 63c1560..ae4dc2e 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -6260,20 +6310,19 @@ mips_print_fp_register (struct ui_file *file, struct frame_info *frame,
>  			int regnum)
>  {				/* Do values for FP (float) regs.  */
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int fpsize = register_size (gdbarch, regnum);
>    gdb_byte *raw_buffer;
>    double doub, flt1;	/* Doubles extracted from raw hex data.  */
>    int inv1, inv2;
>  
> -  raw_buffer
> -    = ((gdb_byte *)
> -       alloca (2 * register_size (gdbarch, mips_regnum (gdbarch)->fp0)));
> +  raw_buffer = alloca (2 * fpsize);

 You need to keep this cast here; have you tried building GDB with a C++ 
compiler?

 You've lost a change to `mips_print_float_info' here -- again, was it by 
accident?

> @@ -8167,6 +8216,7 @@ value_of_mips_user_reg (struct frame_info *frame, const void *baton)
>  static struct gdbarch *
>  mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
> +  struct gdbarch_tdep_info tdep_info = { NULL };
>    struct gdbarch *gdbarch;
>    struct gdbarch_tdep *tdep;
>    int elf_flags;
> @@ -8181,6 +8231,10 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    int dspacc;
>    int dspctl;
>  
> +  /* Wire in an empty template tdep_info if one hasn't been supplied.  */
> +  if (info.tdep_info == NULL)
> +    info.tdep_info = &tdep_info;
> +
>    /* Fill in the OS dependent register numbers and names.  */
>    if (info.osabi == GDB_OSABI_IRIX)
>      {
[...]
> @@ -8311,7 +8366,15 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  	  return NULL;
>  	}
>  
> -      valid_p = 1;
> +      /* Set the floating-point register size, assuming that whoever
> +         supplied the description got the current setting right wrt
> +         CP0 Status register's bit FR if applicable.  */
> +      fpsize = tdesc_register_size (feature, mips_fprs[0]) / 8;
> +
> +      /* Only accept a description whose floating-point register size
> +         matches the requested size or if none was specified.  */
> +      valid_p = (info.tdep_info->fp_register_size == 0
> +		 || info.tdep_info->fp_register_size == fpsize);
>        for (i = 0; i < 32; i++)
>  	valid_p &= tdesc_numbered_register (feature, tdesc_data,
>  					    i + mips_regnum.fp0, mips_fprs[i]);
> @@ -8366,6 +8429,9 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  	    }
>  	}
>  
> +      /* Fix the floating-point register size found.  */
> +      info.tdep_info->fp_register_size = fpsize;
> +
>        /* It would be nice to detect an attempt to use a 64-bit ABI
>  	 when only 32-bit registers are provided.  */
>        reg_names = NULL;
> @@ -8576,6 +8642,11 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>        /* Be pedantic about which FPU is selected.  */
>        if (gdbarch_tdep (arches->gdbarch)->mips_fpu_type != fpu_type)
>  	continue;
> +      /* Ditto the requested floating-point register size if any.  */
> +      if (info.tdep_info->fp_register_size != 0
> +	  && (gdbarch_tdep (arches->gdbarch)->fp_register_size)
> +	      != info.tdep_info->fp_register_size)
> +	continue;
>  
>        if (tdesc_data != NULL)
>  	tdesc_data_cleanup (tdesc_data);
> @@ -8593,6 +8664,8 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    tdep->mips_fpu_type = fpu_type;
>    tdep->register_size_valid_p = 0;
>    tdep->register_size = 0;
> +  tdep->fp_register_size = info.tdep_info->fp_register_size;
> +  tdep->fp_register_size_fixed_p = 0;
>  
>    if (info.target_desc)
>      {
[...]
> @@ -8869,7 +8949,7 @@ mips_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    mips_register_g_packet_guesses (gdbarch);
>  
>    /* Hook in OS ABI-specific overrides, if they have been registered.  */
> -  info.tdep_info = tdesc_data;
> +  ((struct gdbarch_tdep_info*)(info.tdep_info))->tdesc_data = tdesc_data;

 Missing space after the cast again, but why don't you need the same cast 
to access `info.tdep_info->fp_register_size' above?  Have you verified 
this change actually builds?

 Overall again I think it'll be best refactored to avoid these casts, e.g. 
rename the existing null `tdep_info' to `null_tdep_info', and then add a 
new `tdep_info' variable to keep a correctly typed pointer and use it 
throughout.

> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index f0ba0cf..c3405b6 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -534,16 +534,35 @@ get_thread_arch_regcache (ptid_t ptid, struct gdbarch *gdbarch)
>  static ptid_t current_thread_ptid;
>  static struct gdbarch *current_thread_arch;
>  
> -struct regcache *
> -get_thread_regcache (ptid_t ptid)
> +static void
> +set_current_thread_ptid_arch (ptid_t ptid)
>  {
>    if (!current_thread_arch || !ptid_equal (current_thread_ptid, ptid))
>      {
>        current_thread_ptid = ptid;
>        current_thread_arch = target_thread_architecture (ptid);
>      }
> +}
> +
> +struct regcache *
> +get_thread_regcache (ptid_t ptid)
> +{
> +  int registers_changed_p = current_regcache == NULL;
> +  struct regcache *new_regcache;
> +
> +  set_current_thread_ptid_arch (ptid);
> +  new_regcache = get_thread_arch_regcache (ptid, current_thread_arch);
> +
> +  if (registers_changed_p
> +      && gdbarch_regcache_changed_p (current_thread_arch)
> +      && gdbarch_regcache_changed (current_thread_arch, new_regcache))
> +    {
> +      registers_changed ();
> +      set_current_thread_ptid_arch (ptid);
> +      new_regcache = get_thread_arch_regcache (ptid, current_thread_arch);
> +    }
>  
> -  return get_thread_arch_regcache (ptid, current_thread_arch);
> +  return new_regcache;
>  }
>  
>  struct regcache *

 As per <https://sourceware.org/ml/gdb-patches/2012-06/msg00291.html> this 
part requires a general maintainer's approval.

 ISTR though I hit issues with core file handling triggered by this part 
of the change, only observed after the original patch submission (and 
likely the reason I didn't just ping the patch).  So while this patch adds 
new functionality and therefore does not have to have each and every case 
handled right away from the beginning, please double-check it does not 
break things, and in particular does not cause a crash while handling a 
core file.

 Also a similar wrapper around `get_thread_arch_regcache' may be needed 
for places that call it directly rather than via `get_thread_regcache'.  
Please investigate.

  Maciej