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

Siva Chandra
2014-10-20T19:56:41Z

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.