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

[Alan Hayward <Alan.Hayward@arm.com>]

> On 7 Aug 2018, at 17:05, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> On 2018-08-07 07:01, Alan Hayward wrote:
>> It’s probably clearer if I explain the SVE specific case:
>> With SVE when we read the section from the core file it will give us
>> one of two things:
>> 1) SVE header followed by a full SVE register dump (size dependant on
>> register size)
>> Or
>> 2) SVE header followed by a neon register dump (I usually refer to
>> this as a fpsimd dump).
>> The second is a shortcut used by the kernel if the process hadn't run
>> any SVE code, and
>> all the SVE state is null. This structure is significantly smaller
>> than the SVE dump.
>> In the common gdb supply code, it will assert if the size of the
>> section read from the core
>> file is smaller than the given size. So for SVE I need to set the size
>> to the fpsimd size
>> to prevent this.
>> Ok, I could peak ahead into the core file to see what type of dump it
>> is. But, at the
>> point of the _iterate_over_regset_sections() we don’t have any access
>> to the core file.
>> In the common collect code, it uses the size to allocate memory for
>> writing the dump into.
>> For this we always want to write out a full SVE dump, so need the size
>> of the first dump.
>> If that makes sense now, I can rework it into the comments.
> 
> Ok, it kind of make sense but I still find the naming (min_size and size) confusing.  I can't quickly tell what is used when.

Agreed.

> 
> From what I understand, when supplying, you only care about min_size.  If REGSET_VARIABLE_SIZE is not set, min_size is actually not a min_size, but the expected size (anything else than this size will be rejected).  When collecting, you only care about "size", to allocate the memory for the dump.
> 

Yes.

> 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.


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?


> 
> In core_target::get_core_register_section (or maybe somewhere else), we can assert that
> 
>  if (!variable_size_section)
>    gdb_assert (supply_size == collect_size);
> 

Will do.


> 
> 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.


Alan.