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

[will schmidt via Gdb-patches <gdb-patches@sourceware.org>]

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