[PATCH 1/4] gdb/riscv: remove extra caching of misa register
Palmer Dabbelt
palmer@sifive.com
Wed Aug 29 23:10:00 GMT 2018
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,
More information about the Gdb-patches
mailing list