This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 1/3] Add min size to regset section iterations
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