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

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

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.

 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.

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.

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);


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.

Simon