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

[Siva Chandra <sivachandra at google dot com>]

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.