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

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

[Siva Chandra]

This patch series enables having chained function calls in
expressions. An example of a chained function call is shown in PR
c++/11606. It has an example of a chain of two function calls. This
patch series enables chains of any number of function calls.

Currently, an inferior function call is handled via
call_function_by_hand. The value returned by the inferior function is
copied into a GDB value whose lval_type is not_lval. Its contents are
stored within the value irrespective of whether the return value is in
inferior memory or in a register. Consequently, any subsequent
function call in the expression which requires this value's address as
an argument throws an error as the value is not in inferior memory.

This patch series keeps most of the current flow intact, except that
the value returned by the inferior function is made to be a new
lval_type called lval_mirrored_on_inferior_stack. These values have a
mirrored value of lval_type lval_memory which reside on the inferior
stack. They reside on the stack only for the duration for which the
expression is evaluated. This enables value_address to return the
address of the stack mirror instead of throwing an error.

Patch 1/2 - Adds new lval_type named lval_mirrored_on_inferior_stack.
Also adds support for values with this lval_type.
Patch 2/2 - Enables chained function calls by mirroring values
returned by inferior functions in the inferior stack.

Patch 2/2 only targets values returned by call_function_by_hand. I
think similar things can done for call_internal_function and
call_xmethod. I will extend the idea to these functions as well after
this patch series is approved (if at all).

Thanks,
Siva Chandra

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

[Siva Chandra]

On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
> This patch series enables having chained function calls in
> expressions. An example of a chained function call is shown in PR
> c++/11606. It has an example of a chain of two function calls. This
> patch series enables chains of any number of function calls.
>
> Currently, an inferior function call is handled via
> call_function_by_hand. The value returned by the inferior function is
> copied into a GDB value whose lval_type is not_lval. Its contents are
> stored within the value irrespective of whether the return value is in
> inferior memory or in a register. Consequently, any subsequent
> function call in the expression which requires this value's address as
> an argument throws an error as the value is not in inferior memory.
>
> This patch series keeps most of the current flow intact, except that
> the value returned by the inferior function is made to be a new
> lval_type called lval_mirrored_on_inferior_stack. These values have a
> mirrored value of lval_type lval_memory which reside on the inferior
> stack. They reside on the stack only for the duration for which the
> expression is evaluated. This enables value_address to return the
> address of the stack mirror instead of throwing an error.
>
> Patch 1/2 - Adds new lval_type named lval_mirrored_on_inferior_stack.
> Also adds support for values with this lval_type.
> Patch 2/2 - Enables chained function calls by mirroring values
> returned by inferior functions in the inferior stack.
>
> Patch 2/2 only targets values returned by call_function_by_hand. I
> think similar things can done for call_internal_function and
> call_xmethod. I will extend the idea to these functions as well after
> this patch series is approved (if at all).

I used global state in patch 2/2. I thought eliminating that would not
be a straightforward task. However, I spent time looking into it and
it turned out to be much simpler than I had anticipated. I have now
updated both 1/2 and 2/2 to not use any global state. I have also
regression tested and found that a known failure now passes. Will
follow up with v2 of 1/2 and 2/2 both.

Thanks,
Siva Chandra

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

[Ulrich Weigand]

> On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
> > This patch series enables having chained function calls in
> > expressions. An example of a chained function call is shown in PR
> > c++/11606. It has an example of a chain of two function calls. This
> > patch series enables chains of any number of function calls.
> >
> > Currently, an inferior function call is handled via
> > call_function_by_hand. The value returned by the inferior function is
> > copied into a GDB value whose lval_type is not_lval. Its contents are
> > stored within the value irrespective of whether the return value is in
> > inferior memory or in a register. Consequently, any subsequent
> > function call in the expression which requires this value's address as
> > an argument throws an error as the value is not in inferior memory.
> >
> > This patch series keeps most of the current flow intact, except that
> > the value returned by the inferior function is made to be a new
> > lval_type called lval_mirrored_on_inferior_stack. These values have a
> > mirrored value of lval_type lval_memory which reside on the inferior
> > stack. They reside on the stack only for the duration for which the
> > expression is evaluated. This enables value_address to return the
> > address of the stack mirror instead of throwing an error.

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

Bye,
Ulrich

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

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

[Siva Chandra]

On Wed, Oct 1, 2014 at 6:15 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
>> > This patch series enables having chained function calls in
>> > expressions. An example of a chained function call is shown in PR
>> > c++/11606. It has an example of a chain of two function calls. This
>> > patch series enables chains of any number of function calls.
>> >
>> > Currently, an inferior function call is handled via
>> > call_function_by_hand. The value returned by the inferior function is
>> > copied into a GDB value whose lval_type is not_lval. Its contents are
>> > stored within the value irrespective of whether the return value is in
>> > inferior memory or in a register. Consequently, any subsequent
>> > function call in the expression which requires this value's address as
>> > an argument throws an error as the value is not in inferior memory.
>> >
>> > This patch series keeps most of the current flow intact, except that
>> > the value returned by the inferior function is made to be a new
>> > lval_type called lval_mirrored_on_inferior_stack. These values have a
>> > mirrored value of lval_type lval_memory which reside on the inferior
>> > stack. They reside on the stack only for the duration for which the
>> > expression is evaluated. This enables value_address to return the
>> > address of the stack mirror instead of throwing an error.
>
> 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.

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.

Thanks,
Siva Chandra

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

[Siva Chandra]

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:
>>> On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
>>> > This patch series enables having chained function calls in
>>> > expressions. An example of a chained function call is shown in PR
>>> > c++/11606. It has an example of a chain of two function calls. This
>>> > patch series enables chains of any number of function calls.
>>> >
>>> > Currently, an inferior function call is handled via
>>> > call_function_by_hand. The value returned by the inferior function is
>>> > copied into a GDB value whose lval_type is not_lval. Its contents are
>>> > stored within the value irrespective of whether the return value is in
>>> > inferior memory or in a register. Consequently, any subsequent
>>> > function call in the expression which requires this value's address as
>>> > an argument throws an error as the value is not in inferior memory.
>>> >
>>> > This patch series keeps most of the current flow intact, except that
>>> > the value returned by the inferior function is made to be a new
>>> > lval_type called lval_mirrored_on_inferior_stack. These values have a
>>> > mirrored value of lval_type lval_memory which reside on the inferior
>>> > stack. They reside on the stack only for the duration for which the
>>> > expression is evaluated. This enables value_address to return the
>>> > address of the stack mirror instead of throwing an error.
>>
>> 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.
>
> 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.

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.

Thanks,
Siva Chandra

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

[Ulrich Weigand]

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

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

[Siva Chandra]

On Mon, Oct 20, 2014 at 9:01 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> However, I'm still not convinced that the decision ought to made at the
> point a function *returns*.

I am not sure any decision is being made here. All my patch set does
is to make temporary copies of return values on the stack.

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

I am confused about what you are describing. As far as mechanics of
function calls goes, references are just pointers:
http://mentorembedded.github.io/cxx-abi/abi.html#reference-parameter

And, that is how GDB treats reference arguments any way. But ...

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

 ... for this case, as you point out, a temporary on the stack has to
be created and its address has to be passed as argument. GDB is broken
for this case currently and my patch set does not fix it.

> 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?  */

I have not thought enough about "any" function argument in general as
it can in general be a bad thing. But I agree that there could be
exceptions made for cases like this. I can take this up as a follow up
task after this patch set is either accepted or abandoned.

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

How do we distinguish between which func arg can be copied on to the
stack and which cannot? May be we could, but GDB already creates
temporary copies for objects returned by value when the ABI requires
that the return value's address be passed as a hidden first arg. My
patch set actually derives inspiration from this; why not treat all
return values like this instead of only when the ABI requires it?

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

I agree that the new lval type is not required in principle. However,
I brought them in so that it is clear as to how they have actually
come to exist in that way at all. I can remove them it if it seems
like noise.

> (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?)

Ah, Catch! It seems like I generated the diff from a wrong commit.
Will shortly send the v3 of the patch set which has this correctly
handled.

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

I will add a comment for this. However, I am not sure how an
expression evaluation (involving inferior function calls) can be
performed when the inferior thread is actually running.

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

Thanks a lot for actually taking time to look at this. The gymnastics
of this patch set should not be required iff and when the 'compile'
command can get going for C++. Until then, it will be great if we can
get GDB to do "obvious" things correctly.

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

[Ulrich Weigand]

Siva Chandra wrote:
> On Mon, Oct 20, 2014 at 9:01 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > 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.
> 
> I am confused about what you are describing. As far as mechanics of
> function calls goes, references are just pointers:
> http://mentorembedded.github.io/cxx-abi/abi.html#reference-parameter

I was not refering to the ABI, but the C++ standard semantics that
define what happen when you pass an *object* as argument to a function
that expects a *reference* parameter.  See e.g.:
http://en.cppreference.com/w/cpp/language/reference_initialization

   References are initialized in the following situations: 
   [...]
   3) In a function call expression, when the function parameter
      has reference type
   [...]
   The effects of reference initialization are: 
   [...]
   if the reference is [...] lvalue reference to const: 
   [...]
   a temporary of type T is constructed and copy-initialized from
   object. The reference is then bound to this temporary

[ B.t.w. note that for full compatibility with C++, we might also need
to invoke the *constructor* when creating the temporary, not just the
destructor afterwards. ]

> > 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.
> 
>  ... for this case, as you point out, a temporary on the stack has to
> be created and its address has to be passed as argument. GDB is broken
> for this case currently and my patch set does not fix it.

My point was that if you'd implemented construction of the temporary *there*,
then the patch would handle that case as well, without needing two separate
implementations.

> > 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?  */
> 
> I have not thought enough about "any" function argument in general as
> it can in general be a bad thing. But I agree that there could be
> exceptions made for cases like this. I can take this up as a follow up
> task after this patch set is either accepted or abandoned.

The C++ standard says that if the function accepts a const reference,
then *any* object of that type can be passed, and will always lead to
creation of a temporary (unless the object is already an lvalue).

This should make it safe to do the same in GDB as well.

> > 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.
> 
> How do we distinguish between which func arg can be copied on to the
> stack and which cannot? 

As mentioned above, it should always be safe if we have a const reference.
(And if we don't, the call *should* fail even if we pass the return value
of another function call, since it wouldn't be allowed in C++ either.)

> May be we could, but GDB already creates
> temporary copies for objects returned by value when the ABI requires
> that the return value's address be passed as a hidden first arg. My
> patch set actually derives inspiration from this; why not treat all
> return values like this instead of only when the ABI requires it?

I really feel this attempts to address the problem at the wrong point.
Not only do you not handle arguments that aren't function return values,
it would be difficult to get constructor calls correct (see above), and
you create mirrored values even for return values that are *not* used
as function arguments subsequently ...

> > 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.
> 
> I agree that the new lval type is not required in principle. However,
> I brought them in so that it is clear as to how they have actually
> come to exist in that way at all. I can remove them it if it seems
> like noise.

Yes, I think it would be better to just use lval_memory.  There isn't really
anything special about those values, *except* that might have to clean them
up later; and for that, we have to keep them on a separate list anyway.

[ As an aside, I'm wondering whether it wouldn't be cleaner to keep that list
of on-stack temporaries associated with the current *thread* instead of an
expression; it might be more natural since they reside on the thread stack,
and call_function_by_hand already uses the thread struct, so no need to
pass an expression all the way down into it.  Also, infrun then might
verify that temporaries were indeed cleaned up before restarting execution
of the thread (for anything except inferior calls).  ]

> > (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?)
> 
> Ah, Catch! It seems like I generated the diff from a wrong commit.
> Will shortly send the v3 of the patch set which has this correctly
> handled.

Note that 2/2 of the v3 seems to be missing the actual patch ...

> > 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 ...
> 
> I will add a comment for this. However, I am not sure how an
> expression evaluation (involving inferior function calls) can be
> performed when the inferior thread is actually running.

As I said, I agree that this cannot really happen with current code.
 
> > 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!
> 
> Thanks a lot for actually taking time to look at this. The gymnastics
> of this patch set should not be required iff and when the 'compile'
> command can get going for C++. Until then, it will be great if we can
> get GDB to do "obvious" things correctly.

Agreed.

Thanks,
Ulrich

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

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

[Siva Chandra]

On Tue, Oct 21, 2014 at 4:15 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> I was not refering to the ABI, but the C++ standard semantics that
> define what happen when you pass an *object* as argument to a function
> that expects a *reference* parameter.  See e.g.:
> http://en.cppreference.com/w/cpp/language/reference_initialization
>
>    References are initialized in the following situations:
>    [...]
>    3) In a function call expression, when the function parameter
>       has reference type
>    [...]
>    The effects of reference initialization are:
>    [...]
>    if the reference is [...] lvalue reference to const:
>    [...]
>    a temporary of type T is constructed and copy-initialized from
>    object. The reference is then bound to this temporary

I do not think applies in general for const references. IIUC, it
applies to prvalues/literals [section 12.2 in the C++ std]. For all
other kind of values, the two cases above this point should be
applied.

(Moreover, if it applies to all const reference arguments, what
benefit are references bringing in over value arguments? One could
just pass const value args.)

> [ B.t.w. note that for full compatibility with C++, we might also need
> to invoke the *constructor* when creating the temporary, not just the
> destructor afterwards. ]

This is another reason why targeting return values is simpler: we do
not need to construct anything (via a constructor) as the inferior
function would have constructed it appropriately. We only need to
invoke the destructor on these objects.

In the current state of the union however:

1. GDB does not distinguish between const and non-const reference
arguments. IMO, this is semantically OK as the compiler would have
ensured the inferior function does not modify values pointed to by
const-references.

2. For value arguments which need to be passed as a reference (when
the ABI determines so), GDB still does what it does in #1 above. This
IMO is wrong in the case on non-const value arguments as non-const
value arguments can be written to in the called inferior function. The
correct fix for this would be to invoke the constructor and
destructor. (At the very least, make a bit copy on the stack which
will not need construction or destruction, but see point 3 below.)

3. For values passed by value, GDB does not invoke a copy constructor,
but only does a bit copy. Consequently, invocation of a destructor is
also not required. I think this is OK for 99% of the use cases, but
could be fixed for the remaining cases.

4. GDB does not invoke destructors on function return values returned
by value. This is not OK as it can potentially leak inferior memory.

>> > 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.
>>
>>  ... for this case, as you point out, a temporary on the stack has to
>> be created and its address has to be passed as argument. GDB is broken
>> for this case currently and my patch set does not fix it.
>
> My point was that if you'd implemented construction of the temporary *there*,
> then the patch would handle that case as well, without needing two separate
> implementations.
>
>> > 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?  */
>>
>> I have not thought enough about "any" function argument in general as
>> it can in general be a bad thing. But I agree that there could be
>> exceptions made for cases like this. I can take this up as a follow up
>> task after this patch set is either accepted or abandoned.
>
> The C++ standard says that if the function accepts a const reference,
> then *any* object of that type can be passed, and will always lead to
> creation of a temporary (unless the object is already an lvalue).

Function arguments could be xvalues. IIUC, temporaries are created
only for prvalues. You can point me to the appropriate section in the
C++ standard if I am wrong.

> This should make it safe to do the same in GDB as well.
>
>> > 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.
>>
>> How do we distinguish between which func arg can be copied on to the
>> stack and which cannot?
>
> As mentioned above, it should always be safe if we have a const reference.
> (And if we don't, the call *should* fail even if we pass the return value
> of another function call, since it wouldn't be allowed in C++ either.)

Functions can have value arguments and can also return by value.  This
piece of code is valid C++ without involving const references in a
chained function call:

class A
{
public:
  int a;
  A () { }
  A (const A &obj) { a = obj.a; }
  int geta (void);
};

int
A::geta ()
{
  return a;
}

A
func (A a)
{
  A n;

  n.a = a.a + 1000;

  return n;
}

int
main (void)
{
  A a;

  a.a = 0;
  a.a = func (func (func (a))).geta();  /* function chain  */

  return 0;
}

>> May be we could, but GDB already creates
>> temporary copies for objects returned by value when the ABI requires
>> that the return value's address be passed as a hidden first arg. My
>> patch set actually derives inspiration from this; why not treat all
>> return values like this instead of only when the ABI requires it?
>
> I really feel this attempts to address the problem at the wrong point.
> Not only do you not handle arguments that aren't function return values,
> it would be difficult to get constructor calls correct (see above), and

As I mentioned above, return values will not need a constructor invocation.

> you create mirrored values even for return values that are *not* used  I think
> as function arguments subsequently ...

1. The last return value is mirrored on the stack even if not used as
an argument to a subsequent function call. But, this is necessary
anyway. We clean it up after the expression is evaluated.
2. Intermediate non-class(struct/union) return values also get
mirrored on stack even though they might not be used as a function
args. But, since expressions are actually "interpreted" rather than
"compiled" from within GDB, there is no way to determine which need a
copy, which do not.

>> > 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.
>>
>> I agree that the new lval type is not required in principle. However,
>> I brought them in so that it is clear as to how they have actually
>> come to exist in that way at all. I can remove them it if it seems
>> like noise.
>
> Yes, I think it would be better to just use lval_memory.  There isn't really
> anything special about those values, *except* that might have to clean them
> up later; and for that, we have to keep them on a separate list anyway.

OK. I will remove the new lval type.

> [ As an aside, I'm wondering whether it wouldn't be cleaner to keep that list
> of on-stack temporaries associated with the current *thread* instead of an
> expression; it might be more natural since they reside on the thread stack,
> and call_function_by_hand already uses the thread struct, so no need to
> pass an expression all the way down into it.  Also, infrun then might
> verify that temporaries were indeed cleaned up before restarting execution
> of the thread (for anything except inferior calls).  ]
>
>> > (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?)
>>
>> Ah, Catch! It seems like I generated the diff from a wrong commit.
>> Will shortly send the v3 of the patch set which has this correctly
>> handled.
>
> Note that 2/2 of the v3 seems to be missing the actual patch ...

Sent it now.

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

[Siva Chandra]

On Tue, Oct 21, 2014 at 1:30 PM, Siva Chandra <sivachandra@google.com> wrote:
> On Tue, Oct 21, 2014 at 4:15 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> I was not refering to the ABI, but the C++ standard semantics that
>> define what happen when you pass an *object* as argument to a function
>> that expects a *reference* parameter.  See e.g.:
>> http://en.cppreference.com/w/cpp/language/reference_initialization
>>
>>    References are initialized in the following situations:
>>    [...]
>>    3) In a function call expression, when the function parameter
>>       has reference type
>>    [...]
>>    The effects of reference initialization are:
>>    [...]
>>    if the reference is [...] lvalue reference to const:
>>    [...]
>>    a temporary of type T is constructed and copy-initialized from
>>    object. The reference is then bound to this temporary
>
> I do not think applies in general for const references. IIUC, it
> applies to prvalues/literals [section 12.2 in the C++ std]. For all
> other kind of values, the two cases above this point should be
> applied.

I should have said prvalues of non-class type.

> Function arguments could be xvalues. IIUC, temporaries are created
> only for prvalues. You can point me to the appropriate section in the
> C++ standard if I am wrong.

Same here.

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

[Ulrich Weigand]

Siva Chandra wrote:
> On Tue, Oct 21, 2014 at 4:15 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> > I was not refering to the ABI, but the C++ standard semantics that
> > define what happen when you pass an *object* as argument to a function
> > that expects a *reference* parameter.  See e.g.:
> > http://en.cppreference.com/w/cpp/language/reference_initialization
> >
> >    References are initialized in the following situations:
> >    [...]
> >    3) In a function call expression, when the function parameter
> >       has reference type
> >    [...]
> >    The effects of reference initialization are:
> >    [...]
> >    if the reference is [...] lvalue reference to const:
> >    [...]
> >    a temporary of type T is constructed and copy-initialized from
> >    object. The reference is then bound to this temporary
> 
> I do not think applies in general for const references. IIUC, it
> applies to prvalues/literals [section 12.2 in the C++ std]. For all
> other kind of values, the two cases above this point should be
> applied.

> I should have said prvalues of non-class type.

You're right that class and non-class types need to be distinguished,
sorry about the confusion.   [ It seems this is an area that changed
between C++98 and the current standard; in C++98 the implementation
would have been free to chose to either bind directly to the rvalue
or else construct a temporary. ]

In any case, there are still at least some cases where we should create
temporaries at the point an argument is passed.  But -as discussed
below- you're right that there are other cases where we need to create
a temporary to hold a function return value.  So I guess in GDB the
infrastructure we're about to set up should allow for creation and
tracking of temporaries for either function arguments or return values.

> > [ B.t.w. note that for full compatibility with C++, we might also need
> > to invoke the *constructor* when creating the temporary, not just the
> > destructor afterwards. ]
> 
> This is another reason why targeting return values is simpler: we do
> not need to construct anything (via a constructor) as the inferior
> function would have constructed it appropriately. We only need to
> invoke the destructor on these objects.

That's another good point I missed.  You've now convinced me we definitely
need to track return values, in those cases where they were constructed by
the inferior according to the ABI.  (Which should be more-or-less the
cases where language_pass_by_reference returns true.)

Those instances should then result in GDB value objects that point to the
on-stack temporary; and if such objects are passed to further calls, that
on-stack temporary should be used directly.

I'm still not quite sure about the remaining cases (e.g. class types with
no constructors that are returned e.g. in registers according to the ABI);
it still seems preferable to me to have those pushed to the stack only
at the time they are used as arguments to another call.

> In the current state of the union however:
> 
> 1. GDB does not distinguish between const and non-const reference
> arguments. IMO, this is semantically OK as the compiler would have
> ensured the inferior function does not modify values pointed to by
> const-references.

Well, the only point in making a distinction in GDB would be to
error out when the user attempts to make an inferior call using
an argument that would be invalid in C++ (e.g. passing a prvalue
to a non-const reference argument).

> 2. For value arguments which need to be passed as a reference (when
> the ABI determines so), GDB still does what it does in #1 above. This
> IMO is wrong in the case on non-const value arguments as non-const
> value arguments can be written to in the called inferior function. The
> correct fix for this would be to invoke the constructor and
> destructor. (At the very least, make a bit copy on the stack which
> will not need construction or destruction, but see point 3 below.)

Agreed.

> 3. For values passed by value, GDB does not invoke a copy constructor,
> but only does a bit copy. Consequently, invocation of a destructor is
> also not required. I think this is OK for 99% of the use cases, but
> could be fixed for the remaining cases.

Hmmm, I thought the ABI requires the use of a temporary in all cases
where a non-trivial copy constructor exists, so this shouldn't be an
issue?

> 4. GDB does not invoke destructors on function return values returned
> by value. This is not OK as it can potentially leak inferior memory.

Agreed.

[ Snipped some more arguments why we need to track temporaries for
return values, since you've already convinced me :-) ]

> > you create mirrored values even for return values that are *not* used  I think
> > as function arguments subsequently ...
> 
> 1. The last return value is mirrored on the stack even if not used as
> an argument to a subsequent function call. But, this is necessary
> anyway. We clean it up after the expression is evaluated.
> 2. Intermediate non-class(struct/union) return values also get
> mirrored on stack even though they might not be used as a function
> args. But, since expressions are actually "interpreted" rather than
> "compiled" from within GDB, there is no way to determine which need a
> copy, which do not.

Well, the way to determine whether we need to copy in those cases would
be to actually perform the copy at the point the value is used as
*argument*.  As I understand, those are exactly the types of values
that would be handled if we allowed creation of temporaries when
passing a non-class prvalue as const reference.  [ Passing as value
should be fine for such types anyway.  ]

So in summary, what looks the best approach to me right now would be:

- Do something along the lines of your patch (with the changes already
  discussed) to preserve return value temporaries as lval_memory objects
  in those cases where they are mandated by the ABI (i.e. for types that
  have a non-trivial copy constructor or destructor).

- (As in your patch) Keep those values on a list, and once the current
  expression evaluation is finished, invoke destructors and convert the
  value to non_lval if necessary.

- Add code to create temporaries when passing non_lval values of types
  without non-trivial copy constructors or destructors as const references.

- Add code to invoke copy constructors when passing objects by value via
  temporary as mandated by the ABI.  Keep such temporaries on the list
  for the destructors to be called later.

Of course, all that can be done in stages.

Does this look reasonable now or am I still missing something?

Bye,
Ulrich

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

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

[Siva Chandra]

I agree with everything you have said and listed. I have one last point to make.

On Wed, Oct 22, 2014 at 9:59 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> - Do something along the lines of your patch (with the changes already
>   discussed) to preserve return value temporaries as lval_memory objects
>   in those cases where they are mandated by the ABI (i.e. for types that
>   have a non-trivial copy constructor or destructor).

Restricting to just those values whose address is returned in a hidden
param (per ABI) is OK, but an implementation detail made me to include
all types of return values. There are two times when a return value's
address is required:

1. When a method is invoked on the return value: GDB has to evaluate
the 'this' pointer. This is done in eval.c.
2. When the return value is in turn passed as a parameter to a
function expecting a reference: GDB has to evaluate the address of the
value and generate a TYPE_CODE_REF value from it. This logic is in
'value_arg_coerce' in infcall.c.

For objects returned in registers, having logic to create a copy on
the stack for situations like 2 is fine. However, for situations like
1, I do not think there is any way to tell whether one is dealing with
an intermediate return value or not. In which case, we are hit by the
comment at eval.c:1405. This is the reason why I have return values
returned in registers copied on to the stack in my patch. Note that,
doing a bit copy of objects returned in registers should be just fine
as they do not have non-trivial copy constructors or destructors (if
they do, then ABI says that they wont be returned in registers).

One way to get around the comment at eval.c:1405 is to make copies of
only those objects which are not_lval and which do not have
non-trivial copy-ctors or d-tors. The same thing has be to done for 2
anyway if we take this route, meaning that we change the flow in two
places (infcall.c and eval.c). If you feel that this is OK, I will
take this route. My intention was to keep the existing logic as is for
as much as possible. If we keep what I have in my patch, then we only
need to change 'value_arg_coerce' in infrun.c (and that too, only to
enable passing non-lvalue parameters to functions expecting
const-reference arguments).

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

[Ulrich Weigand]

Siva Chandra wrote:

> Restricting to just those values whose address is returned in a hidden
> param (per ABI) is OK, but an implementation detail made me to include
> all types of return values. There are two times when a return value's
> address is required:
> 
> 1. When a method is invoked on the return value: GDB has to evaluate
> the 'this' pointer. This is done in eval.c.

Ah, I knew there was something else I was missing :-)

In this case, I agree with the rest of your explanation.  Please add a
comment in the final version of the patch that explains why the extra
copy is necessary.

There's just one minor change I think would be good: can we allocate that
extra copy on the stack at least only for *class* types, then?  I'd prefer
to avoid this for the common case of routines just returning a scalar.

Thanks,
Ulrich

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

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

[Siva Chandra]

On Thu, Oct 23, 2014 at 9:09 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> In this case, I agree with the rest of your explanation.  Please add a
> comment in the final version of the patch that explains why the extra
> copy is necessary.
>
> There's just one minor change I think would be good: can we allocate that
> extra copy on the stack at least only for *class* types, then?  I'd prefer
> to avoid this for the common case of routines just returning a scalar.

I will soon send a new version of the patch with all the suggested
changes in this thread.

Thanks,
Siva Chandra

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

[Siva Chandra]

On Tue, Sep 30, 2014 at 5:42 PM, Siva Chandra <sivachandra@google.com> wrote:
> On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
>> This patch series enables having chained function calls in
>> expressions. An example of a chained function call is shown in PR
>> c++/11606. It has an example of a chain of two function calls. This
>> patch series enables chains of any number of function calls.
>>
>> Currently, an inferior function call is handled via
>> call_function_by_hand. The value returned by the inferior function is
>> copied into a GDB value whose lval_type is not_lval. Its contents are
>> stored within the value irrespective of whether the return value is in
>> inferior memory or in a register. Consequently, any subsequent
>> function call in the expression which requires this value's address as
>> an argument throws an error as the value is not in inferior memory.
>>
>> This patch series keeps most of the current flow intact, except that
>> the value returned by the inferior function is made to be a new
>> lval_type called lval_mirrored_on_inferior_stack. These values have a
>> mirrored value of lval_type lval_memory which reside on the inferior
>> stack. They reside on the stack only for the duration for which the
>> expression is evaluated. This enables value_address to return the
>> address of the stack mirror instead of throwing an error.
>>
>> Patch 1/2 - Adds new lval_type named lval_mirrored_on_inferior_stack.
>> Also adds support for values with this lval_type.
>> Patch 2/2 - Enables chained function calls by mirroring values
>> returned by inferior functions in the inferior stack.
>>
>> Patch 2/2 only targets values returned by call_function_by_hand. I
>> think similar things can done for call_internal_function and
>> call_xmethod. I will extend the idea to these functions as well after
>> this patch series is approved (if at all).
>
> I used global state in patch 2/2. I thought eliminating that would not
> be a straightforward task. However, I spent time looking into it and
> it turned out to be much simpler than I had anticipated. I have now
> updated both 1/2 and 2/2 to not use any global state. I have also
> regression tested and found that a known failure now passes. Will
> follow up with v2 of 1/2 and 2/2 both.

Ping. Links to the other patches in this series:

https://sourceware.org/ml/gdb-patches/2014-10/msg00001.html
https://sourceware.org/ml/gdb-patches/2014-10/msg00002.html

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

[Siva Chandra]

On Wed, Oct 8, 2014 at 5:02 PM, Siva Chandra <sivachandra@google.com> wrote:
> On Tue, Sep 30, 2014 at 5:42 PM, Siva Chandra <sivachandra@google.com> wrote:
>> On Fri, Sep 26, 2014 at 6:29 AM, Siva Chandra <sivachandra@google.com> wrote:
>>> This patch series enables having chained function calls in
>>> expressions. An example of a chained function call is shown in PR
>>> c++/11606. It has an example of a chain of two function calls. This
>>> patch series enables chains of any number of function calls.
>>>
>>> Currently, an inferior function call is handled via
>>> call_function_by_hand. The value returned by the inferior function is
>>> copied into a GDB value whose lval_type is not_lval. Its contents are
>>> stored within the value irrespective of whether the return value is in
>>> inferior memory or in a register. Consequently, any subsequent
>>> function call in the expression which requires this value's address as
>>> an argument throws an error as the value is not in inferior memory.
>>>
>>> This patch series keeps most of the current flow intact, except that
>>> the value returned by the inferior function is made to be a new
>>> lval_type called lval_mirrored_on_inferior_stack. These values have a
>>> mirrored value of lval_type lval_memory which reside on the inferior
>>> stack. They reside on the stack only for the duration for which the
>>> expression is evaluated. This enables value_address to return the
>>> address of the stack mirror instead of throwing an error.
>>>
>>> Patch 1/2 - Adds new lval_type named lval_mirrored_on_inferior_stack.
>>> Also adds support for values with this lval_type.
>>> Patch 2/2 - Enables chained function calls by mirroring values
>>> returned by inferior functions in the inferior stack.
>>>
>>> Patch 2/2 only targets values returned by call_function_by_hand. I
>>> think similar things can done for call_internal_function and
>>> call_xmethod. I will extend the idea to these functions as well after
>>> this patch series is approved (if at all).
>>
>> I used global state in patch 2/2. I thought eliminating that would not
>> be a straightforward task. However, I spent time looking into it and
>> it turned out to be much simpler than I had anticipated. I have now
>> updated both 1/2 and 2/2 to not use any global state. I have also
>> regression tested and found that a known failure now passes. Will
>> follow up with v2 of 1/2 and 2/2 both.
>
> Ping. Links to the other patches in this series:
>
> https://sourceware.org/ml/gdb-patches/2014-10/msg00001.html
> https://sourceware.org/ml/gdb-patches/2014-10/msg00002.html

Ping.