Re: [PATCH 0/2] Make chained function calls in expressions work

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Siva Chandra wrote:
> On Wed, Oct 1, 2014 at 11:05 AM, Siva Chandra <sivachandra@google.com> wrote:
> > On Wed, Oct 1, 2014 at 6:15 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> >> I'm wondering if there isn't a simpler way to solve this issue: couldn't
> >> you instead during preparation of the second call_function_by_hand simply
> >> allocate extra space on the stack and copy not_lval values whose address
> >> needs to be taken there?   This would avoid adding the new lval type, all
> >> the extra state to track mirrored values during an expression, and would
> >> actually allow you to pass *other* not_lval values to inferior calls too
> >> (not just those originating from another inferior call).
> >
> > I did think about this route. However, look at the comment at
> > eval.c:1405. It has an argument for why we should not in general copy
> > function args on to the stack.

Good point, we cannot do that in the general case.

> > My patches here target return values of functions. Though return
> > values end up being function arguments in a chained function call
> > expression, IMO return values do not suffer from the same problem
> > pointed to in the comment from above.
> >
> > 1. If a function returns a reference, creating a copy of the reference
> > on the stack and passing it around for the duration the expression is
> > being evaluated should not be a problem.
> >
> > 2. If a function returns a value, then it is either returned on the
> > stack or in a register. My patches do not really disturb the case of a
> > value being returned on stack. Even when values are returned in
> > registers, intermediate return values are only temporaries and holding
> > onto their addresses in some stateful entity will be an error.

However, I'm still not convinced that the decision ought to made at the
point a function *returns*.  As I understand the C++ standard, creation
of the temporary is rather triggered by the fact that a value is bound
to a function *argument* of reference type.

For example, if we had a function taking a "const int &" argument, then
passing an integer constant to that function should result in the creation
of a temporary, even though there is no function return value involved.

It seems to me the proper place to handle this would be the TYPE_CODE_REF
case in value_arg_coerce, where we currently have this comment:

        /* Cast the value to the reference's target type, and then
           convert it back to a reference.  This will issue an error
           if the value was not previously in memory - in some cases
           we should clearly be allowing this, but how?  */

> A tangential point, GDB does not call destructors on these temporaries
> which IMO is an error. That is why, in my 2/2, you will notice that
> the expression's state is holding onto all the return value
> temporaries in a vector instead of just the last one created; In a
> future pass, I would like to implement invoking the destructors on
> these temporaries after the expression result is evaluated.

I see.  This is indeed a good argument for temporaries to persist longer
than a single inferior call.  We could still *allocate* (and initialize)
the temporary when processing function arguments for the "outer" call,
though, see above.  (As an aside, the precise rules on temporary
creation and lifetime are of course language specific.  So in an ideal
world, we might want the language parser to handle this and insert
temporary creation/destruction opcodes into the expression, and have
expression evaluation simply process those as usual without having
to hard-code C++ semantics.  However, as there's unfortunately already
lots of other C++ semantics hardcoded, this is certainly not a hard
requirement to get the patch accepted at this point ...)


Another point: Even if we allow for temporaries to persist, I'm still
not sure I understand the need for the new lval_mirrored_on_inferior_stack
type.  Why can't they just be regular (non-lazy) lval_memory values?  Those
already both refer to an inferior address and cache the contents in GDB
memory.  At the point the temporaries in the inferior disappear, those
values could then be flipped to non_lval.  (Actually, I'm not seeing
anywhere in your current patches where lval_mirrored_on_inferior_stack
values are marked invalid once the inferior stack contents disappear?)


A final concern is that leaving temporaries on the stack after an inferior
call is finishes just seems a bit fragile, in particular since they're
*below* the thread's SP value at times, making them potentially vulnerable
to being overwritten by signal handlers etc.  Now I *think* this is not
a problem in current GDB (with your patch) since we will not be running
the inferior thread before expression evaluation ends, *except* for
executing further inferior calls, and those will again adjust the SP.
But future modifications to epression evaluation logic will have to
keep this in mind, so there should at least be a comment somewhere ...

Bye,
Ulrich

PS: Sorry for the delay in replying, I was out of office for a bit and
then got sidetracked with other stuff.  I'll try to be more prompt in
further discussion on this topic.  I certainly appreciate you working
on this; improving GDB's C++ debugging capabilities is important!

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com