diff mbox

[v4] Make chained function calls in expressions work

Message ID CAGyQ6gxhqVQW6eF-ay1UfcW2WgPoa=6=0DJ70dDeWEMJq9By2A@mail.gmail.com
State New
Headers show

Commit Message

Siva Chandra Reddy Oct. 25, 2014, 2:24 p.m. UTC
This is a follow up to the thread here:
https://sourceware.org/ml/gdb-patches/2014-10/msg00000.html

I have made all the suggested changes which now eliminates the need
for two patches in this set. The single patch is attached.

gdb/ChangeLog:

2014-10-25  Siva Chandra Reddy  <sivachandra@google.com>

        * eval.c (evaluate_expression): Cleanup stack mirrors that might
        have been created.
        (add_value_to_expression_stack, skip_current_expression_stack):
        New functions.
        * expression.h (struct expression): New field
        'on_stack_temporaries_vec'.
        * gdbtypes.c (class_or_union_p): New function.
        * gdbtypes.h (class_or_union_p): Declare.
        * infcall.c (call_function_by_hand): New argument EXP of type
        struct expression *. Setup stack temporaries for return values of
        class type.
        (get_return_value_from_memory): New function.
        * infcall.h (call_function_by_hand): Update signature and all
        callers.
        * valarith.c (value_x_binop): New argument EXP of type
        struct expression *.
        (value_x_unop): Likewise.
        * value.c (write_value_to_memory): New function.
        (value_fetch_lazy): Fix its description comment.
        * value.h (add_value_to_expression_stack)
        (skip_current_expression_stack, write_value_to_memory): Declare.
        (value_x_binop, value_x_unop): Update signature and all callers.

gdb/testsuite/ChangeLog:

2014-10-25  Siva Chandra Reddy  <sivachandra@google.com>

        * gdb.cp/chained-calls.cc: New file.
        * gdb.cp/chained-calls.exp: New file.
        * gdb.cp/smartp.exp: Remove KFAIL from c2->inta.

Comments

Siva Chandra Reddy Nov. 3, 2014, 2:35 p.m. UTC | #1
On Sat, Oct 25, 2014 at 7:24 AM, Siva Chandra <sivachandra@google.com> wrote:
> This is a follow up to the thread here:
> https://sourceware.org/ml/gdb-patches/2014-10/msg00000.html
>
> I have made all the suggested changes which now eliminates the need
> for two patches in this set. The single patch is attached.
>
> gdb/ChangeLog:
>
> 2014-10-25  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * eval.c (evaluate_expression): Cleanup stack mirrors that might
>         have been created.
>         (add_value_to_expression_stack, skip_current_expression_stack):
>         New functions.
>         * expression.h (struct expression): New field
>         'on_stack_temporaries_vec'.
>         * gdbtypes.c (class_or_union_p): New function.
>         * gdbtypes.h (class_or_union_p): Declare.
>         * infcall.c (call_function_by_hand): New argument EXP of type
>         struct expression *. Setup stack temporaries for return values of
>         class type.
>         (get_return_value_from_memory): New function.
>         * infcall.h (call_function_by_hand): Update signature and all
>         callers.
>         * valarith.c (value_x_binop): New argument EXP of type
>         struct expression *.
>         (value_x_unop): Likewise.
>         * value.c (write_value_to_memory): New function.
>         (value_fetch_lazy): Fix its description comment.
>         * value.h (add_value_to_expression_stack)
>         (skip_current_expression_stack, write_value_to_memory): Declare.
>         (value_x_binop, value_x_unop): Update signature and all callers.
>
> gdb/testsuite/ChangeLog:
>
> 2014-10-25  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * gdb.cp/chained-calls.cc: New file.
>         * gdb.cp/chained-calls.exp: New file.
>         * gdb.cp/smartp.exp: Remove KFAIL from c2->inta.

Ping.
Siva Chandra Reddy Nov. 3, 2014, 7:55 p.m. UTC | #2
On Mon, Nov 3, 2014 at 6:43 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> struct value *
>> evaluate_expression (struct expression *exp)
>> {
>>+  int i, pc = 0;
>>+  struct value *res, *val;
>>+  struct cleanup *cleanups;
>>+  value_vec *vec = exp->on_stack_temporaries_vec;
>>+
>>+  cleanups = make_cleanup (VEC_cleanup (value_ptr),
>>+                         &exp->on_stack_temporaries_vec);
>>+  res = evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
>>+  /* If the result is on the expression stack, fetch it and mark it as
>>+     not_lval.  */
>>+  for (i = 0; VEC_iterate (value_ptr, vec, 1, val); i++)
>>+    {
>>+      if (res == val)
>>+      {
>>+        if (value_lazy (res))
>>+          value_fetch_lazy (res);
>>+        VALUE_LVAL (res) = not_lval;
>>+        break;
>>+      }
>>+    }
>>+  do_cleanups (cleanups);
>
> There's two issues I see with that:
>
> The minor issue is that modifying a value's lval status by simply assigning
> to VALUE_LVAL has been deprecated for a long time.  While this is another one
> of those partial transformations in GDB code, it would still be better to
> avoid making it worse; if we do need to modify a value's lval, it ought to
> be done via a defined interface implemented in value.c (the implementation
> can simply assign to ->lval direcly).  See also below ...
>
> The more significant problem is that this code is unsafe as is, in the sense
> that it may happen that a temporary is allocated on the stack, but the
> cleanup above never runs for it.  The problem is that all the subroutines
> of evaluate_subexp will pass an expression to call_function_by_hand, but
> only if evaluate_subexp was called via evaluate_expression will the cleanup
> be performed.
>
> Unfortunately, there are several code paths where evaluate_subexp or one of
> its subroutines is called directly.  For example, there's print_object_command
> in objc-lang.c, there's fetch_subexp_value (used from watchpoint code), and
> there is even stap_evaluate_probe_argument calling evaluate_subexp_standard
> directly (I may have missed some further calls).
>
> This is critical not just to make the sure the values are made non_lval
> (on in the future, have destructors called), but also to make sure the
> exp->on_stack_temporaries_vec field is cleared -- if we were to evalutate
> that expression a second time, we'd have invalid values in that vector:
> they might have already been deleted and we access invalid memory, or even
> if not, the skip_current_expression_stack may come up with completely wrong
> SP values (say, from a different thread's stack).
>
> The question is how to the fix that.  One way would be to enfore a rule that
> expression evaluation must go through one (or a set of) particular routines
> and fix all callers that currently violate that rule.  (For example, one
> could imagine doing the cleanup in evaluate_subexp whenever called with
> pc == 0, and changing stap_evaluate_probe_argument to call evaluate_subexp
> instead of evaluate_subexp_standard.)  There is a certain risk of future
> changes re-introducing violations of that rule if it cannot be enforced
> by the compiler ...
>
> Another way might be to continue allowing calls into any subroutine of
> expression evaluation, but set things up so that call_function_by_hand
> will only allow creation of stack temporaries *if* the call came through
> evaluate_expression.  This would require evaluate_expression to take
> some action to enable temporary creation, which could e.g. take the
> form of setting a flag in the expression struct, allocating some other
> structure to hold temporary data and install it into a pointer in the
> expression struct, or even storing the information elsewhere (like in
> the thread struct).

I knew this was coming ... :)

One thing that I did think about was to put this code that I have in
evaluate_expression in evaluate_subexp under if (*pos == 0). This
would also require that we have a flag in struct expression to
indicate to call_function_by_hand that we are evaluating a real/full
expression and that it needs to store temporary values on the stack.
This will make all callers of evaluate_subexp (including
evaluate_expression and evaluate_type) getting the temporary
management right. But, I am not sure if this is complete. WDYT?

>>+/* Return true is T is a class or a union.  False otherwise.  */
>>+
>>+int
>>+class_or_union_p (const struct type *t)
>>+{
>>+  return (TYPE_CODE (t) == TYPE_CODE_STRUCT
>>+        || TYPE_CODE (t) == TYPE_CODE_UNION);
>>+}
>
> I understand we need to do this for classes with member functions (so that
> f().g() will work) -- do we really need it for classes without member
> functions (or plain C structs)?

We could have a struct like this in C++:

struct Derived : public virtual Base
{
 ...
};

Do you mean we should have a language check before reserving space on the stack?

>>@@ -532,6 +563,16 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
>>   {
>>     CORE_ADDR old_sp = get_frame_sp (frame);
>>
>>+    /* Skip over the stack mirrors that might have been generated during the
>>+       evaluation of the current expression.  */
>>+    if (exp != NULL)
>>+      {
>>+      if (gdbarch_inner_than (gdbarch, 1, 2))
>>+        old_sp = skip_current_expression_stack (exp, old_sp, 1);
>>+      else
>>+        old_sp = skip_current_expression_stack (exp, old_sp, 0);
>>+      }
>>+
>>     if (gdbarch_frame_align_p (gdbarch))
>>       {
>>       sp = gdbarch_frame_align (gdbarch, old_sp);
>
> Skipping over existing temporaries *here* may cause the red zone to be added
> a second time on platforms that use it.  This will not necessarily cause any
> problems, but still seems wasteful.  Might be better to skip over temporaries
> *after* the red zone is added, but then need to enforce alignment afterwards
> again.

This is doing a logical "skip" and not really placing the temporaries
before the red zone. Am I missing some other place where red zone is
being "added"? AFAICT, red zone is added right after the above piece
and only at that place. The actuall address of the temporary is
computed further down after the red zone is added and after the
temporaries are skipped.

The only reason I have added the "skip" part here is because the
alignment is done anyway after the red zone is added (or not).

>>@@ -1059,13 +1112,8 @@ When the function is done executing, GDB will silently stop."),
>>        At this stage, leave the RETBUF alone.  */
>>     restore_infcall_control_state (inf_status);
>>
>>-    /* Figure out the value returned by the function.  */
>>-    retval = allocate_value (values_type);
>>-
>>     if (hidden_first_param_p)
>>-      read_value_memory (retval, 0, 1, struct_addr,
>>-                       value_contents_raw (retval),
>>-                       TYPE_LENGTH (values_type));
>>+      retval = get_return_value_from_memory (values_type, struct_addr, exp);
>>     else if (TYPE_CODE (target_values_type) != TYPE_CODE_VOID)
>>       {
>>       /* If the function returns void, don't bother fetching the
>>@@ -1076,16 +1124,30 @@ When the function is done executing, GDB will silently stop."),
>>         case RETURN_VALUE_REGISTER_CONVENTION:
>>         case RETURN_VALUE_ABI_RETURNS_ADDRESS:
>>         case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
>>+          retval = allocate_value (values_type);
>>           gdbarch_return_value (gdbarch, function, values_type,
>>                                 retbuf, value_contents_raw (retval), NULL);
>>+          if (exp != NULL && class_or_union_p (values_type))
>>+            {
>>+              /* Values of class type returned in registers are copied onto
>>+                 the stack and their lval_type set to lval_memory.  This is
>>+                 required because further evaluation of the expression
>>+                 could potentially invoke methods on the return value
>>+                 requiring GDB to evaluate the "this" pointer.  To evaluate
>>+                 the this pointer, GDB needs the memory address of the
>>+                 value.  */
>>+              write_value_to_memory (retval, struct_addr);
>>+              add_value_to_expression_stack (exp, retval);
>>+            }
>>           break;
>>         case RETURN_VALUE_STRUCT_CONVENTION:
>>-          read_value_memory (retval, 0, 1, struct_addr,
>>-                             value_contents_raw (retval),
>>-                             TYPE_LENGTH (values_type));
>>+          retval = get_return_value_from_memory (values_type, struct_addr,
>>+                                                 exp);
>>           break;
>>         }
>>       }
>>+    else
>>+      retval = allocate_value (values_type);
>>
>>     do_cleanups (retbuf_cleanup);
>
> For RETURN_VALUE_ABI_RETURNS_ADDRESS and RETURN_VALUE_ABI_PRESERVES_ADDRESS,
> we in fact have already allocated the return value on the stack (i.e.
> struct_return will be true), so there is no need to write anything back
> to the stack.  This didn't matter with current code, since for those two
> scenarios, gdbarch_return_value will in effect to the same as the
> read_value_memory call in the RETURN_VALUE_STRUCT_CONVENTION case, but
> with your patch, it seems better to avoid the extra store.

This part is confusing to me as, with my little experience with
different archs, I did not really understand the implications of
RETURN_VALUE_ABI_RETURNS_ADDRESS and
RETURN_VALUE_ABI_PRESERVES_ADDRESS. The comments describing them are
confusing (to me atleast) as they talk about return address in
registers et al. But if you say that the following code that you
suggest is good enough, I will go with it.

> Might be simplest to remove the switch (gdbarch_return_value) and just do
>
>   if (TYPE_CODE (values_type) == TYPE_CODE_VOID)
>     retval = allocate_value (values_type);
>   else if (struct_return || hidden_first_param_p)
>     retval = get_return_value_from_memory (values_type, struct_addr, exp);
>   else
>     {
>       retval = allocate_value (values_type);
>       gdbarch_return_value (gdbarch, function, values_type,
>                             retbuf, value_contents_raw (retval), NULL);
>       ...
>     }

...

>>         if (binop_user_defined_p (op, arg1, arg2))
>>-          res_val = value_x_binop (arg1, arg2, op, OP_NULL, EVAL_NORMAL);
>>+          {
>>+            res_val = value_x_binop (arg1, arg2, op, OP_NULL,
>>+                                     EVAL_NORMAL, NULL);
>>+          }
>
> We don't need to add the braces here.

I could be reading wrong, but there is an entry dealing with this
specific point here:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

I will go with your suggestions for other pieces I did not talk about here.

Thanks a lot for reviewing.
Siva Chandra
Siva Chandra Reddy Nov. 3, 2014, 9:22 p.m. UTC | #3
On Mon, Nov 3, 2014 at 11:55 AM, Siva Chandra <sivachandra@google.com> wrote:
>>>+/* Return true is T is a class or a union.  False otherwise.  */
>>>+
>>>+int
>>>+class_or_union_p (const struct type *t)
>>>+{
>>>+  return (TYPE_CODE (t) == TYPE_CODE_STRUCT
>>>+        || TYPE_CODE (t) == TYPE_CODE_UNION);
>>>+}
>>
>> I understand we need to do this for classes with member functions (so that
>> f().g() will work) -- do we really need it for classes without member
>> functions (or plain C structs)?
>
> We could have a struct like this in C++:
>
> struct Derived : public virtual Base
> {
>  ...
> };
>
> Do you mean we should have a language check before reserving space on the stack?

Sorry for revisiting but I thought I can be a bit more elaborate on
this. To complete the example, I am talking about a case like this:

struct Base
{
  int base;
};

struct Derived : public virtual Base
{
  int derived;
};

Derived does not have any methods, but will be returned in a hidden
param as it has a virtual base class. Even for simple structs like
this:

struct Simple
{
  int simple;
};

the return value could be a reference argument for a subsequent
inferior function and hence would still need to be have an address.
Siva Chandra Reddy Nov. 4, 2014, 2:26 p.m. UTC | #4
On Tue, Nov 4, 2014 at 5:38 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Yes, having an additional flag in struct expression would fix the safety
> issue.  Moving initialization to evalute_subexp if *pos == 0 would then
> no longer be safety issue, but simply enabling use of temporaries in more
> cases.

Since I have my code already setup in this fashion, I would prefer to
go this route unless you see an advantage of going with the solution
you suggest below.

> The other option I've been thinking of would be to move the whole handling
> of temporaries to infcall.c, by providing two functions there, e.g. called:
>
>   infcall_enable_on_stack_temporaries ()
>   infcall_cleanup_on_stack_temporaries ()
>
> These would use a new field in the inferior_thread () thread struct to
> store the vector of currently in use on-stack temporaries.  Any call to
> call_function_by_hand after infcall_enable_on_stack_temporaries (and
> before infcall_cleanup_on_stack_temporaries) on the same thread would
> be allowed to use temporaries.  [ There could be sanity checks like
> having call_function_by_hand verify that the current SP still equals
> the thread SP at the time infcall_enable_on_stack_temporaries was
> called.  Also, infrun could assert that a thread that has temporaries
> in use must only ever run during is_infcall, to catch missing cleanup
> calls.  ]
>ReadStringAndDumpToStream<StringElementType::ASCII> (ReadStringAndDumpToStreamOptions options)
> The only thing evaluate_expression would then need to do is to call
> infcall_enable_on_stack_temporaries and install a cleanup calling
> infcall_cleanup_on_stack_temporaries.

[...]

> So I'm thinking of the following sequence of events:
>
> - We call the first call_function_by_hand with a current sp value SP
>   (no pre-existing temporaries at this point).
> - We have to reserve the red zone, setting sp e.g. to SP - 256.
> - We now allocate a temporary, taking up e.g. SP - 256 .. SP - 512.
> - First call_function_by_hand returns.
> - We get a second call_function_by_hand call.
> - Now we have a temporary on the stack from  SP - 256 .. SP - 512,
>   so skip_current_expression_stack skips down to SP - 512.
> - *Now* code allocates the red zone again, setting SP to SP - 768.
>   Even though this is unnecessary; the red zone is actually already
>   there in the range SP ... SP - 256.
>
> Bascially, adding a red zone at any point other the original SP isget_xmethod_arg_types
> useless, since the only point of the red zone is to protect data that
> the original inferior code might have written to its stack immediately
> below the original SP.

Oops! Sorry, I misread my own code. You are right and I will fix it.

> You mean "Any two or more lines in code should be wrapped in braces, even
> if they are comments, as they look like separate statements"?  I don't think
> this is intended to apply to cases like the above where a single statement
> just had to split into multiple lines since it doesn't fit into one.
> This case will never "look like separate statements".  In any case, all
> the existing precedent in GDB does not have extra braces in that case.

Sorry for picking this. I point out because I was asked to put braces
for cases like this in the past. Example:
extension.c:get_xmethod_arg_types.
Siva Chandra Reddy Nov. 4, 2014, 3:08 p.m. UTC | #5
On Tue, Nov 4, 2014 at 5:43 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Well, that's true.  For simple structs, we would have to create a
> temporary at the point of call when using a reference; we discussed
> adding this anyway (it's necessary for scalars as well) ...

I agree for scalars, but is it worth the effort to make this check for
structs/unions? Consider this example:

struct Simpler
{
  char c;
  char getc ();
};

struct Simple
{
  Simpler a, b;
};

Simple
make_simple ()
{
  Simple s;
  s.a.c = 'a';
  s.b.c = 'b';
  return s;
}

Simple does not have any methods, but Simpler has. The ABI returns
objects of Simple type in register. However, one could have an
expression like this: make_simple().a.geta() which is equivalent to
f().g().
Siva Chandra Reddy Nov. 4, 2014, 3:23 p.m. UTC | #6
On Tue, Nov 4, 2014 at 6:58 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> Siva Chandra wrote:
>> On Tue, Nov 4, 2014 at 5:38 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> > Yes, having an additional flag in struct expression would fix the safety
>> > issue.  Moving initialization to evalute_subexp if *pos == 0 would then
>> > no longer be safety issue, but simply enabling use of temporaries in more
>> > cases.
>>
>> Since I have my code already setup in this fashion, I would prefer to
>> go this route unless you see an advantage of going with the solution
>> you suggest below.
>
> Well, I don't think there are any functional advantages as such, but the
> implementation looks a bit cleaner (all the details of temporary handling
> done in infcall.c; fewer interactions between eval.c and infcall.c;
> shorter patch overall).

Shall I go with the thread route then?
Siva Chandra Reddy Nov. 11, 2014, 2:55 p.m. UTC | #7
On Tue, Nov 4, 2014 at 7:40 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> Shall I go with the thread route then?
>
> Yes, please.  Sorry for asking for extra work, but I think overall this
> will be the better implementation.

I spent some time looking into how this can be done. While I do think
it can be done (and should be fairly straight forward), it seemed to
me that it would be more appropriate to cache the temporaries on the
"selected frame". WDYT?

I promise I will not ask any more question after this; I will just
shut up and do whatever you prefer here.
Siva Chandra Reddy Nov. 11, 2014, 3 p.m. UTC | #8
On Tue, Nov 11, 2014 at 6:55 AM, Siva Chandra <sivachandra@google.com> wrote:
> On Tue, Nov 4, 2014 at 7:40 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>>> Shall I go with the thread route then?
>>
>> Yes, please.  Sorry for asking for extra work, but I think overall this
>> will be the better implementation.
>
> I spent some time looking into how this can be done. While I do think
> it can be done (and should be fairly straight forward), it seemed to
> me that it would be more appropriate to cache the temporaries on the
> "selected frame". WDYT?

Or, current frame?

> I promise I will not ask any more question after this; I will just
> shut up and do whatever you prefer here.
Siva Chandra Reddy Nov. 11, 2014, 5:05 p.m. UTC | #9
On Tue, Nov 11, 2014 at 7:21 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> No, please go ahead and ask questions; it's always better to hash the
> main direction out before spending a lot of time on implementating
> something ...  :-)

This time I am really done :)

Thanks for taking time to explain things to me. I am just putting down
my point of view as to why I am still looking for the correct
approach, and after all your explanations, I am now convinced that the
thread approach is the best. One other place where chained function
calls can happen is in Python for example. If val1, val2 and val3 are
gdb.Value objects of type std::string, and if we have val1 + val2 in
Python code, then overloaded operator+ is invoked. However, if we have
val1 + val2 + val3, it will fail currently as this is also a chained
function call. We could perhaps do the 'setup' and 'cleanup' in
ensure_python_env as well (just as we want to do in
evaluate_expression or evaluate_subexp). I do not want to hold up the
current work in relation to this. We can discuss this probably as a
separate topic.
Doug Evans Nov. 12, 2014, 4:08 p.m. UTC | #10
On Tue, Nov 11, 2014 at 7:21 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
> [...]
> The most cleanly "correct" solution would probably be to cache the temporaries
> on the *dummy frame*, because that is the frame associated with all the
> GDB-generated content on the stack, and the dummy_frame struct does persist
> a bit longer than the frame struct.  However, getting there would still be
> a bit more difficult, since we'd first have to arrange for a dummy frame to
> persist over multiple inferior call invocations.  Currently, the dummy frame
> is automatically popped in infrun as soon as the call returns.

Hi.
OOC, Could one do a quick pass over the parsed expression to first see
if there are any function calls, and if so wrap the entire expression
evaluation in a routine that manages the creation/cleanup of an
"outer" dummy frame?

One catch is that if all calls end up being xmethod calls then we
won't need the dummy frame.
I'm not sure if there'd be any real harm in setting one up though.
Doug Evans Nov. 12, 2014, 5:29 p.m. UTC | #11
On Wed, Nov 12, 2014 at 8:08 AM, Doug Evans <dje@google.com> wrote:
> [...]
> One catch is that if all calls end up being xmethod calls then we
> won't need the dummy frame.
> I'm not sure if there'd be any real harm in setting one up though.

Heh, unless one is working with a core file.
Maybe the outer dummy frame could still exist, if in some modified form.
Siva Chandra Reddy Nov. 13, 2014, 3 a.m. UTC | #12
On Wed, Nov 12, 2014 at 8:08 AM, Doug Evans <dje@google.com> wrote:
> On Tue, Nov 11, 2014 at 7:21 AM, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>> [...]
>> The most cleanly "correct" solution would probably be to cache the temporaries
>> on the *dummy frame*, because that is the frame associated with all the
>> GDB-generated content on the stack, and the dummy_frame struct does persist
>> a bit longer than the frame struct.  However, getting there would still be
>> a bit more difficult, since we'd first have to arrange for a dummy frame to
>> persist over multiple inferior call invocations.  Currently, the dummy frame
>> is automatically popped in infrun as soon as the call returns.
>
> Hi.
> OOC, Could one do a quick pass over the parsed expression to first see
> if there are any function calls, and if so wrap the entire expression
> evaluation in a routine that manages the creation/cleanup of an
> "outer" dummy frame?

If I understand you correctly, isn't the general direction of the
proposed solution already this? It differs in the fact that the
"outer" frame is adjusted as required by the expression instead of
"fixing" it before the evaluation of the expression. The notion of an
"outer" frame is not explicitly present though.
diff mbox

Patch

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 5793cd2..32da705 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -10297,7 +10297,7 @@  ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
 		return allocate_value (TYPE_TARGET_TYPE (rtype));
 	      return allocate_value (rtype);
 	    }
-          return call_function_by_hand (argvec[0], nargs, argvec + 1);
+          return call_function_by_hand (argvec[0], nargs, argvec + 1, exp);
 	case TYPE_CODE_INTERNAL_FUNCTION:
 	  if (noside == EVAL_AVOID_SIDE_EFFECTS)
 	    /* We don't know anything about what the internal
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 19aaed3..dee4ab9 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -955,7 +955,7 @@  elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* STT_GNU_IFUNC resolver functions have no parameters.  FUNCTION is the
      function entry address.  ADDRESS may be a function descriptor.  */
 
-  address_val = call_function_by_hand (function, 0, NULL);
+  address_val = call_function_by_hand (function, 0, NULL, NULL);
   address = value_as_address (address_val);
   address = gdbarch_convert_from_func_ptr_addr (gdbarch, address,
 						&current_target);
diff --git a/gdb/eval.c b/gdb/eval.c
index 5906744..d1cc309 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -38,6 +38,7 @@ 
 #include "valprint.h"
 #include "gdb_obstack.h"
 #include "objfiles.h"
+#include "common/vec.h"
 #include <ctype.h>
 
 /* This is defined in valops.c */
@@ -136,9 +137,72 @@  parse_to_comma_and_eval (const char **expp)
 struct value *
 evaluate_expression (struct expression *exp)
 {
-  int pc = 0;
+  int i, pc = 0;
+  struct value *res, *val;
+  struct cleanup *cleanups;
+  value_vec *vec = exp->on_stack_temporaries_vec;
+
+  cleanups = make_cleanup (VEC_cleanup (value_ptr),
+			   &exp->on_stack_temporaries_vec);
+  res = evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
+  /* If the result is on the expression stack, fetch it and mark it as
+     not_lval.  */
+  for (i = 0; VEC_iterate (value_ptr, vec, 1, val); i++)
+    {
+      if (res == val)
+	{
+	  if (value_lazy (res))
+	    value_fetch_lazy (res);
+	  VALUE_LVAL (res) = not_lval;
+	  break;
+	}
+    }
+  do_cleanups (cleanups);
+
+  return res;
+}
+
+/* Add value V to the expression stack of expression EXP.  */
+
+void
+add_value_to_expression_stack (struct expression *exp, struct value *v)
+{
+  gdb_assert (exp != NULL);
+  VEC_safe_push (value_ptr, exp->on_stack_temporaries_vec, v);
+}
+
+/* Return an address after skipping over the current values on the expression
+   stack of EXP.  SP is the current stack frame pointer.  Non-zero DOWNWARD
+   indicates that the stack grows downwards/backwards.  */
 
-  return evaluate_subexp (NULL_TYPE, exp, &pc, EVAL_NORMAL);
+CORE_ADDR
+skip_current_expression_stack (struct expression *exp, CORE_ADDR sp,
+			       int downward)
+{
+  CORE_ADDR addr = sp;
+
+  gdb_assert (exp != NULL);
+  if (!VEC_empty (value_ptr, exp->on_stack_temporaries_vec))
+    {
+      struct value *v = VEC_last (value_ptr, exp->on_stack_temporaries_vec);
+      CORE_ADDR val_addr = value_address (v);
+
+      if (downward)
+	{
+	  gdb_assert (sp >= val_addr);
+	  addr = val_addr;
+	}
+      else
+	{
+	  struct type *type;
+
+	  gdb_assert (sp <= val_addr);
+	  type = value_type (v);
+	  addr = val_addr + TYPE_LENGTH (type);
+	}
+    }
+
+  return addr;
 }
 
 /* Evaluate an expression, avoiding all memory references
@@ -1143,12 +1207,12 @@  evaluate_subexp_standard (struct type *expect_type,
 	argvec[3] = value_from_longest (long_type, selector);
 	argvec[4] = 0;
 
-	ret = call_function_by_hand (argvec[0], 3, argvec + 1);
+	ret = call_function_by_hand (argvec[0], 3, argvec + 1, exp);
 	if (gnu_runtime)
 	  {
 	    /* Function objc_msg_lookup returns a pointer.  */
 	    argvec[0] = ret;
-	    ret = call_function_by_hand (argvec[0], 3, argvec + 1);
+	    ret = call_function_by_hand (argvec[0], 3, argvec + 1, exp);
 	  }
 	if (value_as_long (ret) == 0)
 	  error (_("Target does not respond to this message selector."));
@@ -1165,11 +1229,11 @@  evaluate_subexp_standard (struct type *expect_type,
 	argvec[3] = value_from_longest (long_type, selector);
 	argvec[4] = 0;
 
-	ret = call_function_by_hand (argvec[0], 3, argvec + 1);
+	ret = call_function_by_hand (argvec[0], 3, argvec + 1, exp);
 	if (gnu_runtime)
 	  {
 	    argvec[0] = ret;
-	    ret = call_function_by_hand (argvec[0], 3, argvec + 1);
+	    ret = call_function_by_hand (argvec[0], 3, argvec + 1, exp);
 	  }
 
 	/* ret should now be the selector.  */
@@ -1311,10 +1375,10 @@  evaluate_subexp_standard (struct type *expect_type,
 	    deprecated_set_value_type (argvec[0],
 				       lookup_pointer_type (lookup_function_type (value_type (argvec[0]))));
 	    argvec[0]
-	      = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
+	      = call_function_by_hand (argvec[0], nargs + 2, argvec + 1, exp);
 	  }
 
-	ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1);
+	ret = call_function_by_hand (argvec[0], nargs + 2, argvec + 1, exp);
 	return ret;
       }
       break;
@@ -1431,7 +1495,7 @@  evaluate_subexp_standard (struct type *expect_type,
 		  struct value *value = NULL;
 		  TRY_CATCH (except, RETURN_MASK_ERROR)
 		    {
-		      value = value_x_unop (arg2, op, noside);
+		      value = value_x_unop (arg2, op, noside, exp);
 		    }
 
 		  if (except.reason < 0)
@@ -1734,7 +1798,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	case TYPE_CODE_XMETHOD:
 	  return call_xmethod (argvec[0], nargs, argvec + 1);
 	default:
-	  return call_function_by_hand (argvec[0], nargs, argvec + 1);
+	  return call_function_by_hand (argvec[0], nargs, argvec + 1, exp);
 	}
       /* pai: FIXME save value from call_function_by_hand, then adjust
 	 pc by adjust_fn_pc if +ve.  */
@@ -1845,7 +1909,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	  struct value *value = NULL;
 	  TRY_CATCH (except, RETURN_MASK_ERROR)
 	    {
-	      value = value_x_unop (arg1, op, noside);
+	      value = value_x_unop (arg1, op, noside, exp);
 	    }
 
 	  if (except.reason < 0)
@@ -1945,7 +2009,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else
 	return value_concat (arg1, arg2);
 
@@ -1956,7 +2020,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP || noside == EVAL_AVOID_SIDE_EFFECTS)
 	return arg1;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else
 	return value_assign (arg1, arg2);
 
@@ -1968,7 +2032,8 @@  evaluate_subexp_standard (struct type *expect_type,
 	return arg1;
       op = exp->elts[pc + 1].opcode;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, BINOP_ASSIGN_MODIFY, op, noside);
+	return value_x_binop (arg1, arg2, BINOP_ASSIGN_MODIFY, op, noside,
+			      exp);
       else if (op == BINOP_ADD && ptrmath_type_p (exp->language_defn,
 						  value_type (arg1))
 	       && is_integral_type (value_type (arg2)))
@@ -1999,7 +2064,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else if (ptrmath_type_p (exp->language_defn, value_type (arg1))
 	       && is_integral_type (value_type (arg2)))
 	return value_ptradd (arg1, value_as_long (arg2));
@@ -2018,7 +2083,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else if (ptrmath_type_p (exp->language_defn, value_type (arg1))
 	       && ptrmath_type_p (exp->language_defn, value_type (arg2)))
 	{
@@ -2051,7 +2116,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else
 	{
 	  /* If EVAL_AVOID_SIDE_EFFECTS and we're dividing by zero,
@@ -2094,7 +2159,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
-	return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
       else
 	{
 	  /* If the user attempts to subscript something that is not an
@@ -2160,7 +2225,7 @@  evaluate_subexp_standard (struct type *expect_type,
 
 	  if (binop_user_defined_p (op, arg1, arg2))
 	    {
-	      arg1 = value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	      arg1 = value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	    }
 	  else
 	    {
@@ -2244,7 +2309,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (binop_user_defined_p (op, arg1, arg2))
 	{
 	  arg2 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2271,7 +2336,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (binop_user_defined_p (op, arg1, arg2))
 	{
 	  arg2 = evaluate_subexp (NULL_TYPE, exp, pos, noside);
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2290,7 +2355,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2307,7 +2372,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2324,7 +2389,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2341,7 +2406,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2358,7 +2423,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2375,7 +2440,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	goto nosideret;
       if (binop_user_defined_p (op, arg1, arg2))
 	{
-	  return value_x_binop (arg1, arg2, op, OP_NULL, noside);
+	  return value_x_binop (arg1, arg2, op, OP_NULL, noside, exp);
 	}
       else
 	{
@@ -2410,7 +2475,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (unop_user_defined_p (op, arg1))
-	return value_x_unop (arg1, op, noside);
+	return value_x_unop (arg1, op, noside, exp);
       else
 	{
 	  unop_promote (exp->language_defn, exp->gdbarch, &arg1);
@@ -2422,7 +2487,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (unop_user_defined_p (op, arg1))
-	return value_x_unop (arg1, op, noside);
+	return value_x_unop (arg1, op, noside, exp);
       else
 	{
 	  unop_promote (exp->language_defn, exp->gdbarch, &arg1);
@@ -2437,7 +2502,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (unop_user_defined_p (UNOP_COMPLEMENT, arg1))
-	return value_x_unop (arg1, UNOP_COMPLEMENT, noside);
+	return value_x_unop (arg1, UNOP_COMPLEMENT, noside, exp);
       else
 	{
 	  unop_promote (exp->language_defn, exp->gdbarch, &arg1);
@@ -2449,7 +2514,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (unop_user_defined_p (op, arg1))
-	return value_x_unop (arg1, op, noside);
+	return value_x_unop (arg1, op, noside, exp);
       else
 	{
 	  type = language_bool_type (exp->language_defn, exp->gdbarch);
@@ -2468,7 +2533,7 @@  evaluate_subexp_standard (struct type *expect_type,
       if (noside == EVAL_SKIP)
 	goto nosideret;
       if (unop_user_defined_p (op, arg1))
-	return value_x_unop (arg1, op, noside);
+	return value_x_unop (arg1, op, noside, exp);
       else if (noside == EVAL_AVOID_SIDE_EFFECTS)
 	{
 	  type = check_typedef (value_type (arg1));
@@ -2602,7 +2667,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	return arg1;
       else if (unop_user_defined_p (op, arg1))
 	{
-	  return value_x_unop (arg1, op, noside);
+	  return value_x_unop (arg1, op, noside, exp);
 	}
       else
 	{
@@ -2626,7 +2691,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	return arg1;
       else if (unop_user_defined_p (op, arg1))
 	{
-	  return value_x_unop (arg1, op, noside);
+	  return value_x_unop (arg1, op, noside, exp);
 	}
       else
 	{
@@ -2650,7 +2715,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	return arg1;
       else if (unop_user_defined_p (op, arg1))
 	{
-	  return value_x_unop (arg1, op, noside);
+	  return value_x_unop (arg1, op, noside, exp);
 	}
       else
 	{
@@ -2677,7 +2742,7 @@  evaluate_subexp_standard (struct type *expect_type,
 	return arg1;
       else if (unop_user_defined_p (op, arg1))
 	{
-	  return value_x_unop (arg1, op, noside);
+	  return value_x_unop (arg1, op, noside, exp);
 	}
       else
 	{
@@ -2827,7 +2892,7 @@  evaluate_subexp_for_address (struct expression *exp, int *pos,
       /* We can't optimize out "&*" if there's a user-defined operator*.  */
       if (unop_user_defined_p (op, x))
 	{
-	  x = value_x_unop (x, op, noside);
+	  x = value_x_unop (x, op, noside, exp);
 	  goto default_case_after_eval;
 	}
 
diff --git a/gdb/expression.h b/gdb/expression.h
index 4081a60..05242b6 100644
--- a/gdb/expression.h
+++ b/gdb/expression.h
@@ -76,11 +76,17 @@  union exp_element
     struct objfile *objfile;
   };
 
+struct value;
+typedef struct value *value_ptr;
+DEF_VEC_P (value_ptr);
+typedef VEC (value_ptr) value_vec;
+
 struct expression
   {
     const struct language_defn *language_defn;	/* language it was
 						   entered in.  */
     struct gdbarch *gdbarch;  /* architecture it was parsed in.  */
+    value_vec *on_stack_temporaries_vec;
     int nelts;
     union exp_element elts[1];
   };
diff --git a/gdb/gcore.c b/gdb/gcore.c
index d2adfc8..c4ba961 100644
--- a/gdb/gcore.c
+++ b/gdb/gcore.c
@@ -308,7 +308,7 @@  call_target_sbrk (int sbrk_arg)
   target_sbrk_arg = value_from_longest (builtin_type (gdbarch)->builtin_int, 
 					sbrk_arg);
   gdb_assert (target_sbrk_arg);
-  ret = call_function_by_hand (sbrk_fn, 1, &target_sbrk_arg);
+  ret = call_function_by_hand (sbrk_fn, 1, &target_sbrk_arg, NULL);
   if (ret == NULL)
     return (bfd_vma) 0;
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index ee33d77..5388612 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2501,6 +2501,15 @@  is_scalar_type_recursive (struct type *t)
   return 0;
 }
 
+/* Return true is T is a class or a union.  False otherwise.  */
+
+int
+class_or_union_p (const struct type *t)
+{
+  return (TYPE_CODE (t) == TYPE_CODE_STRUCT
+	  || TYPE_CODE (t) == TYPE_CODE_UNION);
+}
+
 /* A helper function which returns true if types A and B represent the
    "same" class type.  This is true if the types have the same main
    type, or the same name.  */
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index bd1a0ab..f9d2986 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1751,6 +1751,8 @@  extern int get_array_bounds (struct type *type, LONGEST *low_bound,
 
 extern int class_types_same_p (const struct type *, const struct type *);
 
+extern int class_or_union_p (const struct type *);
+
 extern int is_ancestor (struct type *, struct type *);
 
 extern int is_public_ancestor (struct type *, struct type *);
diff --git a/gdb/infcall.c b/gdb/infcall.c
index bbac693..241ec2e 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -455,6 +455,33 @@  cleanup_delete_std_terminate_breakpoint (void *ignore)
   delete_std_terminate_breakpoint ();
 }
 
+/* Reads a value returned by an inferior function for those return
+   values whose address is passed as the hidden first argument.
+   TYPE is the type of value.  ADDR is the address from where to read it.
+   EXP is the expression whose evaluation lead to calling the inferior
+   function.  It is NULL if the inferior function call was not made while
+   evaluating an expression.  */
+
+static struct value *
+get_return_value_from_memory (struct type *type, CORE_ADDR addr,
+			      struct expression *exp)
+{
+  struct value *retval;
+  if (exp != NULL)
+    {
+      retval = value_from_contents_and_address (type, NULL, addr);
+      add_value_to_expression_stack (exp, retval);
+    }
+  else
+    {
+      retval = allocate_value (type);
+      read_value_memory (retval, 0, 1, addr, value_contents_raw (retval),
+			 TYPE_LENGTH (type));
+    }
+
+  return retval;
+}
+
 /* All this stuff with a dummy frame may seem unnecessarily complicated
    (why not just save registers in GDB?).  The purpose of pushing a dummy
    frame which looks just like a real frame is so that if you call a
@@ -471,10 +498,14 @@  cleanup_delete_std_terminate_breakpoint (void *ignore)
    May fail to return, if a breakpoint or signal is hit
    during the execution of the function.
 
-   ARGS is modified to contain coerced values.  */
+   ARGS is modified to contain coerced values.
+   EXP is the expression whose evaluation lead to calling the inferior
+   function.  It is NULL if the inferior function call was not made while
+   evaluating an expression.  */
 
 struct value *
-call_function_by_hand (struct value *function, int nargs, struct value **args)
+call_function_by_hand (struct value *function, int nargs, struct value **args,
+		       struct expression *exp)
 {
   CORE_ADDR sp;
   struct type *values_type, *target_values_type;
@@ -532,6 +563,16 @@  call_function_by_hand (struct value *function, int nargs, struct value **args)
   {
     CORE_ADDR old_sp = get_frame_sp (frame);
 
+    /* Skip over the stack mirrors that might have been generated during the
+       evaluation of the current expression.  */
+    if (exp != NULL)
+      {
+	if (gdbarch_inner_than (gdbarch, 1, 2))
+	  old_sp = skip_current_expression_stack (exp, old_sp, 1);
+	else
+	  old_sp = skip_current_expression_stack (exp, old_sp, 0);
+      }
+
     if (gdbarch_frame_align_p (gdbarch))
       {
 	sp = gdbarch_frame_align (gdbarch, old_sp);
@@ -719,9 +760,21 @@  call_function_by_hand (struct value *function, int nargs, struct value **args)
 
   /* Reserve space for the return structure to be written on the
      stack, if necessary.  Make certain that the value is correctly
-     aligned.  */
+     aligned.
+
+     While evaluating expressions, we reserve space on the stack for
+     return values of class type even if the language ABI and the target
+     ABI do not require that the return value be passed as a hidden first
+     argument.  This is because we want to store the return value as an
+     on-stack temporary while the expression is being evaluated.  This
+     enables us to have chained function calls in expressions.
+
+     Keeping the return values as on-stack temporaries while the expression
+     is being evaluated is OK because the thread is stopped until the
+     expression is completely evaluated.  */
 
-  if (struct_return || hidden_first_param_p)
+  if (struct_return || hidden_first_param_p
+      || (exp != NULL && class_or_union_p (values_type)))
     {
       if (gdbarch_inner_than (gdbarch, 1, 2))
 	{
@@ -1059,13 +1112,8 @@  When the function is done executing, GDB will silently stop."),
        At this stage, leave the RETBUF alone.  */
     restore_infcall_control_state (inf_status);
 
-    /* Figure out the value returned by the function.  */
-    retval = allocate_value (values_type);
-
     if (hidden_first_param_p)
-      read_value_memory (retval, 0, 1, struct_addr,
-			 value_contents_raw (retval),
-			 TYPE_LENGTH (values_type));
+      retval = get_return_value_from_memory (values_type, struct_addr, exp);
     else if (TYPE_CODE (target_values_type) != TYPE_CODE_VOID)
       {
 	/* If the function returns void, don't bother fetching the
@@ -1076,16 +1124,30 @@  When the function is done executing, GDB will silently stop."),
 	  case RETURN_VALUE_REGISTER_CONVENTION:
 	  case RETURN_VALUE_ABI_RETURNS_ADDRESS:
 	  case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
+	    retval = allocate_value (values_type);
 	    gdbarch_return_value (gdbarch, function, values_type,
 				  retbuf, value_contents_raw (retval), NULL);
+	    if (exp != NULL && class_or_union_p (values_type))
+	      {
+		/* Values of class type returned in registers are copied onto
+		   the stack and their lval_type set to lval_memory.  This is
+		   required because further evaluation of the expression
+		   could potentially invoke methods on the return value
+		   requiring GDB to evaluate the "this" pointer.  To evaluate
+		   the this pointer, GDB needs the memory address of the
+		   value.  */
+		write_value_to_memory (retval, struct_addr);
+		add_value_to_expression_stack (exp, retval);
+	      }
 	    break;
 	  case RETURN_VALUE_STRUCT_CONVENTION:
-	    read_value_memory (retval, 0, 1, struct_addr,
-			       value_contents_raw (retval),
-			       TYPE_LENGTH (values_type));
+	    retval = get_return_value_from_memory (values_type, struct_addr,
+						   exp);
 	    break;
 	  }
       }
+    else
+      retval = allocate_value (values_type);
 
     do_cleanups (retbuf_cleanup);
 
diff --git a/gdb/infcall.h b/gdb/infcall.h
index c6dcdc3..4de931b 100644
--- a/gdb/infcall.h
+++ b/gdb/infcall.h
@@ -22,6 +22,7 @@ 
 
 struct value;
 struct type;
+struct expression;
 
 extern CORE_ADDR find_function_addr (struct value *function, 
 				     struct type **retval_type);
@@ -36,6 +37,7 @@  extern CORE_ADDR find_function_addr (struct value *function,
    ARGS is modified to contain coerced values.  */
 
 extern struct value *call_function_by_hand (struct value *function, int nargs,
-					    struct value **args);
+					    struct value **args,
+					    struct expression *exp);
 
 #endif
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c
index 835e612..8ba4741 100644
--- a/gdb/linux-fork.c
+++ b/gdb/linux-fork.c
@@ -468,7 +468,7 @@  inferior_call_waitpid (ptid_t pptid, int pid)
   argv[2] = value_from_longest (builtin_type (gdbarch)->builtin_int, 0);
   argv[3] = 0;
 
-  retv = call_function_by_hand (waitpid_fn, 3, argv);
+  retv = call_function_by_hand (waitpid_fn, 3, argv, NULL);
   if (value_as_long (retv) < 0)
     goto out;
 
@@ -683,7 +683,7 @@  checkpoint_command (char *args, int from_tty)
   old_chain = make_cleanup_restore_integer (&checkpointing_pid);
   checkpointing_pid = ptid_get_pid (inferior_ptid);
 
-  ret = call_function_by_hand (fork_fn, 0, &ret);
+  ret = call_function_by_hand (fork_fn, 0, &ret, NULL);
   do_cleanups (old_chain);
   if (!ret)	/* Probably can't happen.  */
     error (_("checkpoint: call_function_by_hand returned null."));
diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c
index 8a3d21e..10194f0 100644
--- a/gdb/objc-lang.c
+++ b/gdb/objc-lang.c
@@ -131,7 +131,7 @@  lookup_objc_class (struct gdbarch *gdbarch, char *classname)
   classval = value_string (classname, strlen (classname) + 1, char_type);
   classval = value_coerce_array (classval);
   return (CORE_ADDR) value_as_long (call_function_by_hand (function, 
-							   1, &classval));
+							   1, &classval, NULL));
 }
 
 CORE_ADDR
@@ -160,7 +160,7 @@  lookup_child_selector (struct gdbarch *gdbarch, char *selname)
   selstring = value_coerce_array (value_string (selname, 
 						strlen (selname) + 1,
 						char_type));
-  return value_as_long (call_function_by_hand (function, 1, &selstring));
+  return value_as_long (call_function_by_hand (function, 1, &selstring, NULL));
 }
 
 struct value * 
@@ -181,12 +181,12 @@  value_nsstring (struct gdbarch *gdbarch, char *ptr, int len)
   if (lookup_minimal_symbol("_NSNewStringFromCString", 0, 0).minsym)
     {
       function = find_function_in_inferior("_NSNewStringFromCString", NULL);
-      nsstringValue = call_function_by_hand(function, 1, &stringValue[2]);
+      nsstringValue = call_function_by_hand(function, 1, &stringValue[2], NULL);
     }
   else if (lookup_minimal_symbol("istr", 0, 0).minsym)
     {
       function = find_function_in_inferior("istr", NULL);
-      nsstringValue = call_function_by_hand(function, 1, &stringValue[2]);
+      nsstringValue = call_function_by_hand(function, 1, &stringValue[2], NULL);
     }
   else if (lookup_minimal_symbol("+[NSString stringWithCString:]", 0, 0).minsym)
     {
@@ -198,7 +198,7 @@  value_nsstring (struct gdbarch *gdbarch, char *ptr, int len)
 	(type, lookup_objc_class (gdbarch, "NSString"));
       stringValue[1] = value_from_longest 
 	(type, lookup_child_selector (gdbarch, "stringWithCString:"));
-      nsstringValue = call_function_by_hand(function, 3, &stringValue[0]);
+      nsstringValue = call_function_by_hand(function, 3, &stringValue[0], NULL);
     }
   else
     error (_("NSString: internal error -- no way to create new NSString"));
@@ -1194,7 +1194,7 @@  print_object_command (char *args, int from_tty)
   if (function == NULL)
     error (_("Unable to locate _NSPrintForDebugger in child process"));
 
-  description = call_function_by_hand (function, 1, &object);
+  description = call_function_by_hand (function, 1, &object, NULL);
 
   string_addr = value_as_long (description);
   if (string_addr == 0)
diff --git a/gdb/parse.c b/gdb/parse.c
index 27947e7..a03c266 100644
--- a/gdb/parse.c
+++ b/gdb/parse.c
@@ -193,6 +193,7 @@  initialize_expout (struct parser_state *ps, size_t initial_size,
 			+ EXP_ELEM_TO_BYTES (ps->expout_size));
   ps->expout->language_defn = lang;
   ps->expout->gdbarch = gdbarch;
+  ps->expout->on_stack_temporaries_vec = NULL;
 }
 
 /* See definition in parser-defs.h.  */
diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
index 4313170..0d65b48 100644
--- a/gdb/python/py-value.c
+++ b/gdb/python/py-value.c
@@ -795,7 +795,7 @@  valpy_call (PyObject *self, PyObject *args, PyObject *keywords)
       struct cleanup *cleanup = make_cleanup_value_free_to_mark (mark);
       struct value *return_value;
 
-      return_value = call_function_by_hand (function, args_count, vargs);
+      return_value = call_function_by_hand (function, args_count, vargs, NULL);
       result = value_to_value_object (return_value);
       do_cleanups (cleanup);
     }
@@ -1039,7 +1039,10 @@  valpy_binop (enum valpy_opcode opcode, PyObject *self, PyObject *other)
       if (!handled)
 	{
 	  if (binop_user_defined_p (op, arg1, arg2))
-	    res_val = value_x_binop (arg1, arg2, op, OP_NULL, EVAL_NORMAL);
+	    {
+	      res_val = value_x_binop (arg1, arg2, op, OP_NULL,
+				       EVAL_NORMAL, NULL);
+	    }
 	  else
 	    res_val = value_binop (arg1, arg2, op);
 	}
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index 032f5de..f3bd6c7 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -2058,7 +2058,7 @@  flush_ea_cache (void)
       type = lookup_pointer_type (type);
       addr = BMSYMBOL_VALUE_ADDRESS (msymbol);
 
-      call_function_by_hand (value_from_pointer (type, addr), 0, NULL);
+      call_function_by_hand (value_from_pointer (type, addr), 0, NULL, NULL);
     }
 }
 
diff --git a/gdb/testsuite/gdb.cp/chained-calls.cc b/gdb/testsuite/gdb.cp/chained-calls.cc
new file mode 100644
index 0000000..a30ec3b
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/chained-calls.cc
@@ -0,0 +1,203 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+class S
+{
+public:
+  S () { }
+  S (S &obj);
+
+  S operator+ (const S &s);
+
+  int a;
+};
+
+S::S (S &obj)
+{
+  a = obj.a;
+}
+
+S
+S::operator+ (const S &s)
+{
+  S res;
+
+  res.a = a + s.a;
+
+  return res;
+}
+
+S
+f (int i)
+{
+  S s;
+
+  s.a = i;
+
+  return s;
+}
+
+int
+g (const S &s)
+{
+  return s.a;
+}
+
+class A
+{
+public:
+  A operator+ (const A &);
+  int a;
+};
+
+A
+A::operator+ (const A &obj)
+{
+  A n;
+
+  n.a = a + obj.a;
+
+  return n;
+}
+
+A
+p ()
+{
+  A a;
+  a.a = 12345678;
+  return a;
+}
+
+A
+r ()
+{
+  A a;
+  a.a = 10000000;
+  return a;
+}
+
+A
+q (const A &a)
+{
+  return a;
+}
+
+class B
+{
+public:
+  int b[1024];
+};
+
+B
+makeb ()
+{
+  B b;
+  int i;
+
+  for (i = 0; i < 1024; i++)
+    b.b[i] = i;
+
+  return b;
+}
+
+int
+getb (const B &b, int i)
+{
+  return b.b[i];
+}
+
+class C
+{
+public:
+  C ();
+  ~C ();
+
+  A operator* ();
+
+  A *a_ptr;
+};
+
+C::C ()
+{
+  a_ptr = new A;
+  a_ptr->a = 5678;
+}
+
+C::~C ()
+{
+  delete a_ptr;
+}
+
+A
+C::operator* ()
+{
+  return *a_ptr;
+}
+
+#define TYPE_INDEX 1
+
+enum type
+{
+  INT,
+  CHAR
+};
+
+union U
+{
+public:
+  U (type t);
+  type get_type ();
+
+  int a;
+  char c;
+  type tp[2];
+};
+
+U::U (type t)
+{
+  tp[TYPE_INDEX] = t;
+}
+
+U
+make_int ()
+{
+  return U (INT);
+}
+
+U
+make_char ()
+{
+  return U (CHAR);
+}
+
+type
+U::get_type ()
+{
+  return tp[TYPE_INDEX];
+}
+
+int
+main ()
+{
+  int i = g(f(0));
+  A a = q(p() + r());
+
+  B b = makeb ();
+  C c;
+
+  return i + getb(b, 0);  /* Break here  */
+}
diff --git a/gdb/testsuite/gdb.cp/chained-calls.exp b/gdb/testsuite/gdb.cp/chained-calls.exp
new file mode 100644
index 0000000..c903bea
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/chained-calls.exp
@@ -0,0 +1,44 @@ 
+# Copyright 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite
+
+if {[skip_cplus_tests]} { continue }
+
+standard_testfile .cc
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug c++}]} {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "Break here"]
+gdb_continue_to_breakpoint "Break here"
+
+gdb_test "p g(f(12345))" ".* = 12345" "g(f())"
+gdb_test "p q(p())" ".* = {a = 12345678}" "q(p())"
+gdb_test "p p() + r()" ".* = {a = 22345678}" "p() + r()"
+gdb_test "p q(p() + r())" ".* = {a = 22345678}" "q(p() + r())"
+gdb_test "p g(f(6700) + f(89))" ".* = 6789" "g(f() + f())"
+gdb_test "p g(f(g(f(300) + f(40))) + f(5))" ".* = 345" \
+    "g(f(g(f() + f())) + f())"
+gdb_test "p getb(makeb(), 789)" ".* = 789" "getb(makeb(), ...)"
+gdb_test "p *c" ".* = {a = 5678}" "*c"
+gdb_test "p *c + *c" ".* = {a = 11356}" "*c + *c"
+gdb_test "P q(*c + *c)" ".* = {a = 11356}" "q(*c + *c)"
+gdb_test "p make_int().get_type ()" ".* = INT" "make_int().get_type ()"
diff --git a/gdb/testsuite/gdb.cp/smartp.exp b/gdb/testsuite/gdb.cp/smartp.exp
index 2a1028a..e3d271f 100644
--- a/gdb/testsuite/gdb.cp/smartp.exp
+++ b/gdb/testsuite/gdb.cp/smartp.exp
@@ -72,6 +72,5 @@  gdb_test "p b->foo()"         "= 66"
 gdb_test "p c->foo()"         "= 66"
 gdb_test "p c->inta"          "= 77"
 
-setup_kfail "gdb/11606" "*-*-*"
 gdb_test "p c2->inta"          "= 77"
 
diff --git a/gdb/valarith.c b/gdb/valarith.c
index 154629b..d80cf62 100644
--- a/gdb/valarith.c
+++ b/gdb/valarith.c
@@ -339,11 +339,16 @@  value_user_defined_op (struct value **argp, struct value **args, char *name,
 
    OP is the operatore, and if it is BINOP_ASSIGN_MODIFY, then OTHEROP
    is the opcode saying how to modify it.  Otherwise, OTHEROP is
-   unused.  */
+   unused.
+
+   EXP is the expression whose evaluation requires performing the binary
+   operation.  It is NULL if the operation is not being performed as part
+   of an expression evaluation.  */
 
 struct value *
 value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
-	       enum exp_opcode otherop, enum noside noside)
+	       enum exp_opcode otherop, enum noside noside,
+	       struct expression *exp)
 {
   struct value **argvec;
   char *ptr;
@@ -499,12 +504,13 @@  value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
 	}
       else
 	return call_function_by_hand (argvec[0], 2 - static_memfuncp,
-				      argvec + 1);
+				      argvec + 1, exp);
     }
   throw_error (NOT_FOUND_ERROR,
                _("member function %s not found"), tstr);
 #ifdef lint
-  return call_function_by_hand (argvec[0], 2 - static_memfuncp, argvec + 1);
+  return call_function_by_hand (argvec[0], 2 - static_memfuncp, argvec + 1,
+				exp);
 #endif
 }
 
@@ -512,10 +518,15 @@  value_x_binop (struct value *arg1, struct value *arg2, enum exp_opcode op,
    defined operator that matches the operator in question.
    Create an argument vector that calls arg1.operator @ (arg1)
    and return that value (where '@' is (almost) any unary operator which
-   is legal for GNU C++).  */
+   is legal for GNU C++).
+
+   EXP is the expression whose evaluation requires performing the unary
+   operation.  It is NULL if the operation is not being performed as part
+   of an expression evaluation.  */
 
 struct value *
-value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
+value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside,
+	      struct expression *exp)
 {
   struct gdbarch *gdbarch = get_type_arch (value_type (arg1));
   struct value **argvec;
@@ -609,7 +620,7 @@  value_x_unop (struct value *arg1, enum exp_opcode op, enum noside noside)
 	  return call_xmethod (argvec[0], 1, argvec + 1);
 	}
       else
-	return call_function_by_hand (argvec[0], nargs, argvec + 1);
+	return call_function_by_hand (argvec[0], nargs, argvec + 1, exp);
     }
   throw_error (NOT_FOUND_ERROR,
                _("member function %s not found"), tstr);
diff --git a/gdb/valops.c b/gdb/valops.c
index 7f3e4f5..5fcefc2 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -191,7 +191,7 @@  value_allocate_space_in_inferior (int len)
   struct value *blocklen;
 
   blocklen = value_from_longest (builtin_type (gdbarch)->builtin_int, len);
-  val = call_function_by_hand (val, 1, &blocklen);
+  val = call_function_by_hand (val, 1, &blocklen, NULL);
   if (value_logical_not (val))
     {
       if (!target_has_execution)
diff --git a/gdb/value.c b/gdb/value.c
index ecfb154..058d04f 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1080,6 +1080,20 @@  set_value_parent (struct value *value, struct value *parent)
   value_free (old);
 }
 
+/* Write contents of V at ADDR.  Also, set lval_type of V to lval_memory.
+   It is as error if V's lval_type is anything other than not_lval.  */
+
+void
+write_value_to_memory (struct value *v, CORE_ADDR addr)
+{
+  gdb_assert (VALUE_LVAL (v) == not_lval);
+
+  write_memory (addr, value_contents_raw (v), TYPE_LENGTH (value_type (v)));
+  VALUE_LVAL (v) = lval_memory;
+  v->location.address = addr;
+  v->lazy = 0;
+}
+
 gdb_byte *
 value_contents_raw (struct value *value)
 {
@@ -3730,11 +3744,8 @@  value_initialized (struct value *val)
   return val->initialized;
 }
 
-/* Called only from the value_contents and value_contents_all()
-   macros, if the current data for a variable needs to be loaded into
-   value_contents(VAL).  Fetches the data from the user's process, and
-   clears the lazy flag to indicate that the data in the buffer is
-   valid.
+/* Fetches the data from the user's process, and clears the lazy flag
+   to indicate that the data in the buffer is valid.
 
    If the value is zero-length, we avoid calling read_memory, which
    would abort.  We mark the value as fetched anyway -- all 0 bytes of
diff --git a/gdb/value.h b/gdb/value.h
index e3603c3..1e0efee 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -363,6 +363,8 @@  extern const gdb_byte *
 
 extern int value_fetch_lazy (struct value *val);
 
+extern void write_value_to_memory (struct value *v, CORE_ADDR addr);
+
 /* If nonzero, this is the value of a variable which does not actually
    exist in the program, at least partially.  If the value is lazy,
    this may fetch it now.  */
@@ -819,6 +821,12 @@  extern struct value *evaluate_subexp (struct type *expect_type,
 extern struct value *evaluate_subexpression_type (struct expression *exp,
 						  int subexp);
 
+extern void add_value_to_expression_stack (struct expression *exp,
+					   struct value *v);
+
+extern CORE_ADDR skip_current_expression_stack (struct expression *exp,
+						CORE_ADDR sp, int downward);
+
 extern void fetch_subexp_value (struct expression *exp, int *pc,
 				struct value **valp, struct value **resultp,
 				struct value **val_chain,
@@ -939,10 +947,11 @@  extern struct value *value_of_this_silent (const struct language_defn *lang);
 extern struct value *value_x_binop (struct value *arg1, struct value *arg2,
 				    enum exp_opcode op,
 				    enum exp_opcode otherop,
-				    enum noside noside);
+				    enum noside noside,
+				    struct expression *exp);
 
 extern struct value *value_x_unop (struct value *arg1, enum exp_opcode op,
-				   enum noside noside);
+				   enum noside noside, struct expression *exp);
 
 extern struct value *value_fn_field (struct value **arg1p, struct fn_field *f,
 				     int j, struct type *type, int offset);