two agent expression nits (one line each)

[David Taylor]

Joel Brobecker <brobecker@adacore.com> wrote:

> Hi Stan,
> 
> > > The @item line and the following text do no agree with one another.  I'm
> > > guessing that the text is correct, in which case this line:
> > > 
> > >     @item @code{setv} (0x2d) @var{n}: @result{} @var{v}
> > > 
> > > should be changed to this:
> > > 
> > >     @item @code{setv} (0x2d) @var{n}: @var{v} @result{} @var{v}
> > 
> > Yes, that is correct.
> > 
> > > Additionally, in gdb/common/ax.def we find the line:
> > > 
> > >     DEFOP (setv, 2, 0, 0, 1, 0x2d)
> > > 
> > >>From the comment earlier in the file:
> > > 
> > >        Each line is of the form:
> > > 
> > >        DEFOP (name, size, data_size, consumed, produced, opcode)
> > > [...]
> > >        CONSUMED is the number of stack elements consumed.
> > >        PRODUCED is the number of stack elements produced.
> > > 
> > > which is saying that nothing is consumed and one item is produced.  Both
> > > should be 0 or both should be 1.  Setting them both to 1 seems better
> > > since if nothing is on the stack an error will occur.  So, it should be
> > > changed to:
> > > 
> > >     DEFOP (setv, 2, 0, 1, 1, 0x2d)
> > 
> > Yes to this also.  From the look of things, I cloned the getv definition
> > and forgot to adjust it.
> > 
> > Although technically an incompatible change to the bytecode language,
> > anybody who tried to set a state variable within a larger expression was
> > going to get odd behavior and/or stack overflow, while the common
> > case of just setting the variable and discarding the result is
> > unaffected.  So I think we can just make the change directly, and
> > perhaps add a brief note to NEWS, to make sure that target agents
> > get updated. (Mentor has one in-house separate from gdbserver, plus at
> > least two customers with their own agents.)
> 
> Would you mind taking care of that for us? I kept that message around
> in the hope that I'd have a clearer picture with a little extra time,
> but I am still not sure whether you're just suggesting a documentation
> fix, or if you're saying we should fix the implementation...

The first part is pure documentation -- the description is correct but
the summary line is wrong.  The second part is implementation -- the
consumed and produced fields are used to check for stack underflow and
overflow.

The fields are used by gdb and gdbserver for sanity checking the
bytecode expression -- checking for underflow and overflow.

The underflow check catches bad bytecode sequences generated by GDB.

Since DEFOP has consumed as 0 when it is really 1, that means that GDB
will think that there is more on the stack than there really is.

So, if GDB generates a bytecode sequence that underflows it might not
catch it.  It does not currently generate bad sequences, so fixing it
will merely help catch future lossage.

As I recall, the overflow check is used by gdbserver to verify that it
has sufficent stack space for the expression.  The bad DEFOP line makes
it think that the expression is using one more stack slot than reality.
But, gdbserver has fairly generous bytecode stack space -- so unless the
expression is really complicated, that won't happen.

> Thanks!
> -- 
> Joel