Re: [PATCH v2 1/3] Add min size to regset section iterations

[Simon Marchi <simon.marchi@polymtl.ca>]

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