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

will schmidt via Gdb-patches
2022-02-02T01:54:30Z

On Tue, 2022-02-01 at 20:27 -0500, Simon Marchi wrote:
>
> 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.

Ok, Thanks much for the response. I'll dig in again in the
morning and see if I can make more sense of what I'm trying to do. :-)

>
> > 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 don't think so, but we have some details there that I try to be
careful not to trip over. We have some VSX pseudo registers
(vs0..vs63), which overlap our floating point (f0..f31) and altivec
(vr0..vr31) register ranges.

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