From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106774 invoked by alias); 17 Feb 2018 03:53:38 -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 106763 invoked by uid 89); 17 Feb 2018 03:53:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-7.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 17 Feb 2018 03:53:36 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w1H3rUDu001106 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 16 Feb 2018 22:53:34 -0500 Received: by simark.ca (Postfix, from userid 112) id 14A411E76B; Fri, 16 Feb 2018 22:53:30 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 36F061E093; Fri, 16 Feb 2018 22:53:29 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 17 Feb 2018 03:53:00 -0000 From: Simon Marchi To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 00/10 v2] Remove regcache::m_readonly_p In-Reply-To: <1517999572-14987-1-git-send-email-yao.qi@linaro.org> References: <1517999572-14987-1-git-send-email-yao.qi@linaro.org> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.4 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 17 Feb 2018 03:53:30 +0000 X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00228.txt.bz2 On 2018-02-07 05:32, Yao Qi wrote: > regcache is used in many places in gdb in different ways, so regcache > becomes a flat and fat object. That exposes some unnecessary APIs to > different part, and some APIs are misused or abused: > > 1) gdbarch methods pseudo_register_read, pseudo_register_read_value, > read_pc have a parameter 'regcache *', but these two gdbarch methods > only need raw_read* and cooked_read* methods. So it is better to > pass a class which only has raw_read* and cooked_read* methods, and > other regcache methods are invisible to each gdbarch implementation. > > 2) target_ops methods to_fetch_registers and to_store_registers have a > parameter 'regcache *', but these two target_ops methods only need > raw_supply and raw_collect methods, because raw registers are from > target > layer, pseudo registers are "composed" or "created" by gdbarch. > > 3) jit.c uses regcache in an odd way, and record-full.c should use > a simple version regcache instead of an array (see patch 11) > > Beside these api issues, one issue in regcache is that there is no > type or class for readonly regcache. We use a flag field m_readonly_p > to indicate the regcache is readonly or not, so some regcache apis have > assert that this regcache is or is not readonly. The better way to do > this is to create a new class for readonly regcache which doesn't have > write methods at all. > > This patch series fixes all of the problems above except 2) (I had a > patch to fix 2 in my tree, but still need more time to polish it.) by > designing a class hierarchy about regcache, like this, > > reg_buffer > ^ > | > ------+----- > ^ > | > readable_regcache > ^ > | > ------+------ > ^ ^ > | | > detached_regcache readonly_detached_regcache > ^ > | > regcache > > Class reg_buffer is a simple class, having register contents and status > (in patch 1). readable_regcache is an abstract class only having > raw_read* > and cooked_read* methods (in patch 2). detached_regcache is a class > which > has read and write methods, but it detaches from target, IOW, the > write doesn't go through. Class readonly_detached_regcache is the > readonly > regcache, created from regcache::save method. > > This is the v2 of this patch series, v1 can be found > https://sourceware.org/ml/gdb-patches/2017-12/msg00014.html Some > changes compared with v1, > > - Some of the preparatory patches in v1 are already committed, > - Rename some classes, > - Pass readable_regcache to gdbarch read_pc. We can pass > readable_regcache > to gdbarch breakpoint_kind_from_current_state as well, because this > gdbarch method doesn't need to write regcache. The reason I don't > that is arm_breakpoint_kind_from_current_state uses a function > arm_get_next_pcs_ctor shared between GDB and GDBserver, and I don't > propagate the regcache changes to GDBserver at this moment, > > This patch series is pushed to users/qiyao/regcache-split-4-2. > Regression > tested on {x86_64,aarch64}-linux. Hi Yao, I did not spot anything fundamentally wrong in the series, though I'm not very proficient in this area. I sent a few minor comments, but other than that I'm fine with the series. It's hard to tell whether it's the right design, but I think it improves things by splitting the concerns in different classes rather than having one do-it-all class. Thanks, Simon