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

Ulrich Weigand
2014-10-21T11:15:59Z

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