[1/4] gdb/riscv: remove extra caching of misa register

  The RISC-V had a mechanism in place to cache the contents of the misa
register per-inferior, the original intention behind this was to
reduce the number of times the misa register had to be read (as the
contents should be constant), but it was pointed out on the mailing
list[1] that the register cache will mean the register is only
accessed once each time GDB stops, and any additional caching is
probably just unneeded extra complexity.

As such, until it can be shown that there's a real need for additional
caching, this commit removes all of the additional caching of the misa
register, and just accesses the misa register like a normal register.

[1] https://sourceware.org/ml/gdb-patches/2018-03/msg00136.html

gdb/ChangeLog:

	* riscv-tdep.c (struct riscv_inferior_data): Delete.
	(riscv_read_misa_reg): Don't cache value read into inferior data.
	(riscv_new_inferior_data): Delete.
	(riscv_inferior_data_cleanup): Delete.
	(riscv_inferior_data): Delete.
	(riscv_invalidate_inferior_data): Delete.
	(_initialize_riscv_tdep): Remove initialisation of inferior data.
---
 gdb/ChangeLog    |  10 ++++++
 gdb/riscv-tdep.c | 106 +++----------------------------------------------------
 2 files changed, 15 insertions(+), 101 deletions(-)
  

On Wed, 29 Aug 2018 09:40:51 PDT (-0700), andrew.burgess@embecosm.com wrote:
> The RISC-V had a mechanism in place to cache the contents of the misa
> register per-inferior, the original intention behind this was to
> reduce the number of times the misa register had to be read (as the
> contents should be constant), but it was pointed out on the mailing
> list[1] that the register cache will mean the register is only
> accessed once each time GDB stops, and any additional caching is
> probably just unneeded extra complexity.
>
> As such, until it can be shown that there's a real need for additional
> caching, this commit removes all of the additional caching of the misa
> register, and just accesses the misa register like a normal register.
>
> [1] https://sourceware.org/ml/gdb-patches/2018-03/msg00136.html

I like this -- we've got too many special one-off behaviors for registers 
floating around the RISC-V debug stack and I'm not really convinced most of 
them do anything but cause bugs.  I think that unless there's a benchmark that 
demonstrates why they're necessary then we should just nuke the special 
behavior whenever we run into a bug.  Like you said, the generic caches will 
probably soak up the load anyway.

>
> gdb/ChangeLog:
>
> 	* riscv-tdep.c (struct riscv_inferior_data): Delete.
> 	(riscv_read_misa_reg): Don't cache value read into inferior data.
> 	(riscv_new_inferior_data): Delete.
> 	(riscv_inferior_data_cleanup): Delete.
> 	(riscv_inferior_data): Delete.
> 	(riscv_invalidate_inferior_data): Delete.
> 	(_initialize_riscv_tdep): Remove initialisation of inferior data.
> ---
>  gdb/ChangeLog    |  10 ++++++
>  gdb/riscv-tdep.c | 106 +++----------------------------------------------------
>  2 files changed, 15 insertions(+), 101 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 1df1300100c..2f619c35e75 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -60,8 +60,6 @@
>
>  /* Forward declarations.  */
>  static bool riscv_has_feature (struct gdbarch *gdbarch, char feature);
> -struct riscv_inferior_data;
> -struct riscv_inferior_data * riscv_inferior_data (struct inferior *const inf);
>
>  /* Define a series of is_XXX_insn functions to check if the value INSN
>     is an instance of instruction XXX.  */
> @@ -73,22 +71,6 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
>  #include "opcode/riscv-opc.h"
>  #undef DECLARE_INSN
>
> -/* Per inferior information for RiscV.  */
> -
> -struct riscv_inferior_data
> -{
> -  /* True when MISA_VALUE is valid, otherwise false.  */
> -  bool misa_read;
> -
> -  /* If MISA_READ is true then MISA_VALUE holds the value of the MISA
> -     register read from the target.  */
> -  uint32_t misa_value;
> -};
> -
> -/* Key created when the RiscV per-inferior data is registered.  */
> -
> -static const struct inferior_data *riscv_inferior_data_reg;
> -
>  /* Architectural name for core registers.  */
>
>  static const char * const riscv_gdb_reg_names[RISCV_LAST_FP_REGNUM + 1] =
> @@ -293,17 +275,16 @@ static unsigned int riscv_debug_infcall = 0;
>  static uint32_t
>  riscv_read_misa_reg (bool *read_p)
>  {
> -  struct riscv_inferior_data *inf_data
> -    = riscv_inferior_data (current_inferior ());
> +  uint32_t value = 0;
>
> -  if (!inf_data->misa_read && target_has_registers)
> +  if (target_has_registers)
>      {
> -      uint32_t value = 0;
>        struct frame_info *frame = get_current_frame ();
>
>        TRY
>  	{
> -	  value = get_frame_register_unsigned (frame, RISCV_CSR_MISA_REGNUM);
> +	  value = get_frame_register_unsigned (frame,
> +					       RISCV_CSR_MISA_REGNUM);
>  	}
>        CATCH (ex, RETURN_MASK_ERROR)
>  	{
> @@ -312,15 +293,9 @@ riscv_read_misa_reg (bool *read_p)
>  					       RISCV_CSR_LEGACY_MISA_REGNUM);
>  	}
>        END_CATCH
> -
> -      inf_data->misa_read = true;
> -      inf_data->misa_value = value;
>      }
>
> -  if (read_p != nullptr)
> -    *read_p = inf_data->misa_read;
> -
> -  return inf_data->misa_value;
> +  return value;
>  }
>
>  /* Return true if FEATURE is available for the architecture GDBARCH.  The
> @@ -2646,69 +2621,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    return gdbarch;
>  }
>
> -
> -/* Allocate new riscv_inferior_data object.  */
> -
> -static struct riscv_inferior_data *
> -riscv_new_inferior_data (void)
> -{
> -  struct riscv_inferior_data *inf_data
> -    = new (struct riscv_inferior_data);
> -  inf_data->misa_read = false;
> -  return inf_data;
> -}
> -
> -/* Free inferior data.  */
> -
> -static void
> -riscv_inferior_data_cleanup (struct inferior *inf, void *data)
> -{
> -  struct riscv_inferior_data *inf_data =
> -    static_cast <struct riscv_inferior_data *> (data);
> -  delete (inf_data);
> -}
> -
> -/* Return riscv_inferior_data for the given INFERIOR.  If not yet created,
> -   construct it.  */
> -
> -struct riscv_inferior_data *
> -riscv_inferior_data (struct inferior *const inf)
> -{
> -  struct riscv_inferior_data *inf_data;
> -
> -  gdb_assert (inf != NULL);
> -
> -  inf_data
> -    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
> -  if (inf_data == NULL)
> -    {
> -      inf_data = riscv_new_inferior_data ();
> -      set_inferior_data (inf, riscv_inferior_data_reg, inf_data);
> -    }
t
> -
> -  return inf_data;
> -}
> -
> -/* Free the inferior data when an inferior exits.  */
> -
> -static void
> -riscv_invalidate_inferior_data (struct inferior *inf)
> -{
> -  struct riscv_inferior_data *inf_data;
> -
> -  gdb_assert (inf != NULL);
> -
> -  /* Don't call RISCV_INFERIOR_DATA as we don't want to create the data if
> -     we've not already created it by this point.  */
> -  inf_data
> -    = (struct riscv_inferior_data *) inferior_data (inf, riscv_inferior_data_reg);
> -  if (inf_data != NULL)
> -    {
> -      delete (inf_data);
> -      set_inferior_data (inf, riscv_inferior_data_reg, NULL);
> -    }
> -}
> -
>  /* This decodes the current instruction and determines the address of the
>     next instruction.  */
>
> @@ -2853,14 +2765,6 @@ _initialize_riscv_tdep (void)
>  {
>    gdbarch_register (bfd_arch_riscv, riscv_gdbarch_init, NULL);
>
> -  /* Register per-inferior data.  */
> -  riscv_inferior_data_reg
> -    = register_inferior_data_with_cleanup (NULL, riscv_inferior_data_cleanup);
> -
> -  /* Observers used to invalidate the inferior data when needed.  */
> -  gdb::observers::inferior_exit.attach (riscv_invalidate_inferior_data);
> -  gdb::observers::inferior_appeared.attach (riscv_invalidate_inferior_data);
> -
>    /* Add root prefix command for all "set debug riscv" and "show debug
>       riscv" commands.  */
>    add_prefix_cmd ("riscv", no_class, set_debug_riscv_command,