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

[Palmer Dabbelt <palmer@sifive.com>]

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,