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

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

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