From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2554 invoked by alias); 29 Aug 2018 23:10:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 2521 invoked by uid 89); 29 Aug 2018 23:10:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=demonstrates X-HELO: mail-pg1-f182.google.com Received: from mail-pg1-f182.google.com (HELO mail-pg1-f182.google.com) (209.85.215.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Aug 2018 23:09:58 +0000 Received: by mail-pg1-f182.google.com with SMTP id a13-v6so288855pgt.7 for ; Wed, 29 Aug 2018 16:09:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=date:subject:in-reply-to:cc:from:to:message-id:mime-version :content-transfer-encoding; bh=B+21cNfg0uprCMJmBw61SsSyQEWoIgFHxzejMXnXmHU=; b=RMhXTrFU9/g08Iy7KP36IdyyEfbjA37aL0peQt00t9EKLPQzIa66JiSLBGi92Zssok qKJOiRslukOFYqMK1LHrrckcIa2bmrNGJHmVuIlVgwEXRRQnIN+8ZNHyy1BWjYDmu4T8 a6HHWKk7/Jpu6gUSlhKpO8GDF52X5KXtNL0C6Fw6Uropxoxso8cpbfp8m3usnJkzSH0j EaFAkdo9Jo+vDzzp88k/hnFuCCRFEaJqi8Kh2Opp0peh6IYILygDTE6uIjVyJajYNWVU riH1L/oarJb+pBp0sufWa9uGo7j1q6GIz4cZbWNs4zIcxUTSDiR/EZ6l/Ifrut3dYH8j 86Wg== Return-Path: Received: from localhost ([12.206.222.5]) by smtp.gmail.com with ESMTPSA id l3-v6sm7216377pff.8.2018.08.29.16.09.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 29 Aug 2018 16:09:55 -0700 (PDT) Date: Wed, 29 Aug 2018 23:10:00 -0000 X-Google-Original-Date: Wed, 29 Aug 2018 14:53:19 PDT (-0700) Subject: Re: [PATCH 1/4] gdb/riscv: remove extra caching of misa register In-Reply-To: <74dd27a48191cc0cf77fcb241b7463536e1eb4cc.1535560591.git.andrew.burgess@embecosm.com> CC: gdb-patches@sourceware.org, Jim Wilson , andrew.burgess@embecosm.com From: Palmer Dabbelt To: andrew.burgess@embecosm.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2018-08/txt/msg00786.txt.bz2 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 (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,