Re: RFC gdbserver and tdesc and powerpc (stuck on a gdbserver assert)

Simon Marchi via Gdb-patches
2022-02-02T01:27:48Z

On 2022-02-01 16:48, will schmidt via Gdb-patches wrote:
>
> Hi,
> I've been working on a target-description rework for powerpc, this is
> a continuation of work that Rogerio has posted rfc patches for sometime last year.
>
> I've run into a stumbling block with the init_target_desc code in gdbserver,
> and am not sure how to best proceed.
>
> gdbserver/tdesc.cc: init_target_desc(...) iterates through the provided
> features and populates the reg_defs structure. The code currently has an assert
> with a comment:
>
> /* Register number will increase (possibly with gaps) or be zero. */
> gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());
>
> This trips on powerpc (with the WIP tdesc patch set), potentially in several
> locations, since our features contain registers that are intermixed across the
> ranges, so we end up with regnos that numerically belong earlier in the
> tdesc->reg_defs structure, but they belong in the features where they are.
> In particular;
> The Powerpc "core" features includes regnums 0-31 (gprs r0..r31),
> a gap, then 64-69 (PC,MSR,CR,LR,CTR,XER).
> The subsequent "fpu" feature fills in that gap as it includes regnums
> 32-63 (f0..f31), and 70 (fpscr).

From a quick look at the code, my understanding is:

- The order in which the registers appear in the target description
defines the order of the registers in the regcache buffer. See how
`offset` gets incremented for each register in that loop you were
looking at?

- When handling the 'g' packet (read registers), there is currently the
assumption that the order of the registers in the reg_defs vector
(that is, sorted by register number, as reg_defs is indexed by
register number) matches the order of the registers in the regcache
buffer. Look in registers_to_string, how we grab a pointer to the
beginning of the regcache buffer and only advance it as we iterate on
reg_defs.

So, if you were to fill reg_defs in a non-sorted order:

- regnum 0, name=A, size=4
- regnum 2, name=C, size=4
- regnum 1, name=B, size=4

such that you ended up with this reg_defs:

[0] name=A, offset=0
[1] name=B, offset=8
[2] name=C, offset=4

Then registers_to_string would get it wrong, as it would mix up B and C.

I think what you are trying to do is possible, it's just a matter of
either respecting the existing assumptions the code is based on, or
adjust the code to not assume these things anymore. For this specific
problem, I see two possible solutions:

- When filling reg_defs, do a first pass to create all elements. Then
do a second pass, iterating on reg_defs, to fill in the offset
fields.

- Remove the assumption that the order of the registers in the regcache
is the same as in reg_defs. In registers_to_string, that would mean
not advancing a pointer in the regcache buffer, but using each
register's offset to grab the data at the right place in the
regcache buffer. registers_from_string would need to be adapted too,
which seems harder, as it's currently implemented as one big
hex2bin, which assumes the regcache has registers sorted by number.

I would lean towards the first solution, it seems easier.

> There may or may not be an issue with the subsequent altivec and vsx register sets,
> since we have some overlapping ranges there.

Can a single tdesc actually contain two registers with the same number?

> I could split apart the features into smaller bits, but this would scramble the
> documented powerpc features descriptions (as seen in gdb.texinfo).
> I've tried just disabling the assert, but I'm not certain that is sufficient, I currently
> also see some partial transfer errors between gdb and gdbserver that i've not sorted out.

Right after that assert, we have:

/* Register number will increase (possibly with gaps) or be zero. */
gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());

if (regnum != 0)
tdesc->reg_defs.resize (regnum, gdb::reg (offset));

So if you handle registers 0, then 100, then 1, the resize will make it
so the reg_defs vector ends up with a size of 1 (and subsequently 2 when
we emplace_back). Whereas what you want is a vector of size 101 with
all undefined registers except at indices 0, 1 and 100.

So to implement the idea described above, you'd need to change that code
to make sure you only grow the vector.

Simon