From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94857 invoked by alias); 8 Aug 2018 13:34:21 -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 92471 invoked by uid 89); 8 Aug 2018 13:34:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=pragmatic 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; Wed, 08 Aug 2018 13:34:17 +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 w78DYAjV028351 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 8 Aug 2018 09:34:15 -0400 Received: by simark.ca (Postfix, from userid 112) id 81D3E1EF36; Wed, 8 Aug 2018 09:34:10 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 9DAC01E52C; Wed, 8 Aug 2018 09:34:08 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 08 Aug 2018 13:34:00 -0000 From: Simon Marchi To: Alan Hayward Cc: Simon Marchi , GDB Patches , nd Subject: Re: [PATCH v2 1/3] Add min size to regset section iterations In-Reply-To: References: <20180730092528.98739-1-alan.hayward@arm.com> <20180730092528.98739-2-alan.hayward@arm.com> <045ba9b9-6f37-0eeb-5609-f895e29f894f@ericsson.com> <13DC373A-4E12-46C4-AA97-C11E82224056@arm.com> <4caa36e74ca613047db83f1a9868ea3e@polymtl.ca> Message-ID: <85fb7a3bf0d60220a19bab71f49f51cf@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.6 X-IsSubscribed: yes X-SW-Source: 2018-08/txt/msg00154.txt.bz2 On 2018-08-08 04:18, Alan Hayward wrote: >> Therefore, I am starting to think the semantic is more straightforward >> (to me at least) if we named them supply_size and collect_size (which >> you mentioned in the original patch message). This would make it >> somewhat clear that if you are in a supply scenario, collect_size is >> meaningless (and vice versa). It becomes a bit simpler to explain: >> >> - When supplying fixed-size regsets (!REGSET_VARIABLE_SIZE), >> supply_size is the exact expected size. >> - When supplying variable-size regsets (REGSET_VARIABLE_SIZE) >> supply_size is actually just a minumum, because we don't know what we >> will actually find in the section yet. >> - When collecting, we know the size in advance (since we know what we >> will dump), so collect_size is always the exact size that will be >> dumped. > > The only point I have against this is that I had always assumed that > the _iterate_over_regset_sections function was designed so that in the > future extra functions could get added to regset, alongside supply and > collect. If that happened, I expected the new function to use either > size or min size. Calling the sizes collect_size and supply_size would > confuse it. However, I probably shouldn’t worry about that, given it’s > doubtful another function would get added. > > Happy to do it that way. I understand the concern. The min_size/size does indeed sound more generic/extensible, but at the expense of clarity. My pragmatic side prefers supply_size/collect_size, because I think a reader would understand more easily. > But, how about if I moved the two sizes into regset? > > struct regset > { > const void *regmap; > supply_regset_ftype *supply_regset; > int supply_size; > collect_regset_ftype *collect_regset; > int collect_size; > unsigned flags; > }; > > Reducing the callback to: > cb (".reg", &aarch64_linux_gregset, NULL, cb_data); > > For most targets the size will be fixed, so the regset structures can > stay global. > > But I’d have to be careful - for example > s390_iterate_over_regset_sections sets size based on the current abi - > instead I’d create both s390_gregset and s390x_gregset. > > This is why I avoided doing it in the original patch :) > > I could update using your suggestion, then maybe do a follow on patch > with the above? I don't have a strong opinion. It just moves the problem around, passing the info in the structure instead of as formal parameters. I think your original solution is ok, as long as the parameters are clearly documented. Just to put yet another option on the table: since "size" parameter is only used to allocate some space for the collect function to dump the register data in, what about making the collect functions allocate that space themselves. For example, by making them return a gdb::byte_vector. >> On a different track, did you consider keeping a single "size" >> parameter to gdbarch_iterate_over_regset_sections, but add one to >> indicate whether the caller intends to supply or collect registers? >> And then, in aarch64's implementation, pass different sizes in the >> supply/collect cases? Most other arch implementations would simply >> ignore this parameter and always pass the same size, as they do today. > > If it’s going to indicate whether to use supply or collect, then it > would seem odd to pass back a structure with both collect and supply > functions in it, when you know which one isn’t getting used. > > If going down that route, I’d probably split > _iterate_over_regset_sections into two functions, one for collect and > one for supply. And then that gets rid of the regset structure, > replacing it with collect_regset and supply_regset ? At this point it > feels like a large code shuffle. Indeed, I don't think either that's a good direction. Simon