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

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

Thanks for the reviews.

> On 6 Aug 2018, at 19:27, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-07-30 05:25 AM, Alan Hayward wrote:
>> When using the regset section iteration functions, the size parameter is used
>> in different ways.
>> 
>> With collect, size is used to create the buffer in which to write the regset.
>> (see linux-tdep.c::linux_collect_regset_section_cb).
>> 
>> With supply, size is used to confirm the existing regset is the correct size.
>> If REGSET_VARIABLE_SIZE is set then the regset can be bigger than size.
>> Effectively, size is the minimum possible size of the regset.
>> (see corelow.c::get_core_register_section).
>> 
>> There are currently no targets with both REGSET_VARIABLE_SIZE and a collect
>> function.
>> 
>> To allow support of collects for REGSET_VARIABLE_SIZE we need two sizes.
>> Min_size is the minimum allowed size for the regset, and size is used as the
>> size to use when creating new regsets. For all targets that are not
>> REGSET_VARIABLE_SIZE then these two sizes are equal.
> 
> Hi Alan,
> 
> I am still a bit confused by how these two sizes are used.  Please document
> carefully what they each mean, for both the collect and supply case.
> 
> Part of my confusion comes the fact that for Aarch64 SVE, we seem to know
> in advance the exact size the SVE register section should have, based on
> vq (which we read in aarch64_linux_core_read_vq).  In this case, the single
> size parameter would be enough for collecting and supplying, since it's the
> exact size (not a minimum size).
> 

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.

>> diff --git a/gdb/corelow.c b/gdb/corelow.c
>> index 059ce2f6eb..32a054ee3e 100644
>> --- a/gdb/corelow.c
>> +++ b/gdb/corelow.c
>> @@ -107,6 +107,7 @@ public:
>> 				  const struct regset *regset,
>> 				  const char *name,
>> 				  int min_size,
>> +				  int size,
>> 				  int which,
>> 				  const char *human_name,
>> 				  bool required);
>> @@ -570,12 +571,13 @@ core_target::get_core_register_section (struct regcache *regcache,
>> 					const struct regset *regset,
>> 					const char *name,
>> 					int min_size,
>> +					int size,
>> 					int which,
>> 					const char *human_name,
>> 					bool required)
>> {>    struct bfd_section *section;
>> -  bfd_size_type size;
>> +  bfd_size_type core_size;
> 
> I would suggest naming this "section_size", or "reg_section_size".  At first I thought it meant
> the size of the whole core file.  You could push this rename as an obvious patch right now to
> reduce the noise in this patch, since it's a good change on its own.

Agreed. Will do.

> 
> Simon