[PATCH v5] Make chained function calls in expressions work

[PATCH v5] Make chained function calls in expressions work

[Siva Chandra]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

Link to v4: https://sourceware.org/ml/gdb-patches/2014-10/msg00697.html

I have made all the changes suggested by Ulrich Weigand in his comments for v4.

gdb/ChangeLog:

2014-11-16  Siva Chandra Reddy  <sivachandra@google.com>

        * eval.c: Include gdbthread.h.
        (evaluate_subexp): Enable thread stack temporaries before
        evaluating a complete expression and clean them up after the
        evaluation is complete.
        * gdbthread.h: Include common/vec.h.
        (value_ptr): New typedef.
        (VEC (value_ptr)): New vector type.
        (value_vec): New typedef.
        (struct thread_info): Add new fields stack_temporaries_enabled
        and stack_temporaries.
        (enable_thread_stack_temporaries)
        (thread_stack_temporaries_enabled_p, push_thread_stack_temporary)
        (skip_thread_stack_temporaries)
        (value_in_thread_stack_temporaries): Declare.
        * gdbtypes.c (class_or_union_p): New function.
        * gdbtypes.h (class_or_union_p): Declare.
        * infcall.c (get_return_value_from_memory): New function.
        (call_function_by_hand): Store return values of class type as
        temporaries on stack.
        * thread.c (enable_thread_stack_temporaries): New function.
        (thread_stack_temporaries_enabled_p, push_thread_stack_temporary)
        (skip_thread_stack_temporaries): Likewise.
        (value_in_thread_stack_temporaries): Likewise.
        (struct ptid_data): New type.
        * value.c (value_force_lval): New function.
        * value.h (value_force_lval): Declare.

gdb/testsuite/ChangeLog:

2014-11-16  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 for "p c2->inta".

[-- Attachment #2: chained-calls-v5.txt --]
[-- Type: text/plain, Size: 18595 bytes --]

diff --git a/gdb/eval.c b/gdb/eval.c
index 655ea22..be1c6ec 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -24,6 +24,7 @@
 #include "expression.h"
 #include "target.h"
 #include "frame.h"
+#include "gdbthread.h"
 #include "language.h"		/* For CAST_IS_CONVERSION.  */
 #include "f-lang.h"		/* For array bound stuff.  */
 #include "cp-abi.h"
@@ -63,8 +64,28 @@ struct value *
 evaluate_subexp (struct type *expect_type, struct expression *exp,
 		 int *pos, enum noside noside)
 {
-  return (*exp->language_defn->la_exp_desc->evaluate_exp) 
+  struct cleanup *cleanups;
+  struct value *retval;
+  int cleanup_temps = 0;
+
+  if (*pos == 0 && inferior_ptid.pid != 0
+      && exp->language_defn->la_language == language_cplus)
+    {
+      cleanups = enable_thread_stack_temporaries (inferior_ptid);
+      cleanup_temps = 1;
+    }
+
+  retval = (*exp->language_defn->la_exp_desc->evaluate_exp) 
     (expect_type, exp, pos, noside);
+
+  if (cleanup_temps)
+    {
+      if (value_in_thread_stack_temporaries (retval, inferior_ptid))
+	retval = value_non_lval (retval);
+      do_cleanups (cleanups);
+    }
+
+  return retval;
 }
 \f
 /* Parse the string EXP as a C expression, evaluate it,
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 4fd5f69..2bf577c 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -28,6 +28,7 @@ struct symtab;
 #include "ui-out.h"
 #include "inferior.h"
 #include "btrace.h"
+#include "common/vec.h"
 
 /* Frontend view of the thread state.  Possible extensions: stepping,
    finishing, until(ling),...  */
@@ -152,6 +153,10 @@ struct thread_suspend_state
   enum gdb_signal stop_signal;
 };
 
+typedef struct value *value_ptr;
+DEF_VEC_P (value_ptr);
+typedef VEC (value_ptr) value_vec;
+
 struct thread_info
 {
   struct thread_info *next;
@@ -264,6 +269,14 @@ struct thread_info
 
   /* Branch trace information for this thread.  */
   struct btrace_thread_info btrace;
+
+  /* Flag which indicates that the stack temporaries should be stored while
+     evaluating expressions.  */
+  int stack_temporaries_enabled;
+
+  /* Values that are stored as temporaries on stack while evaluating
+     expressions.  */
+  value_vec *stack_temporaries;
 };
 
 /* Create an empty thread list, or empty the existing one.  */
@@ -465,6 +478,16 @@ extern void prune_threads (void);
 
 int pc_in_thread_step_range (CORE_ADDR pc, struct thread_info *thread);
 
+extern struct cleanup *enable_thread_stack_temporaries (ptid_t ptid);
+
+extern int thread_stack_temporaries_enabled_p (ptid_t ptid);
+
+extern void push_thread_stack_temporary (ptid_t ptid, struct value *v);
+
+extern CORE_ADDR skip_thread_stack_temporaries (ptid_t, CORE_ADDR, int);
+
+extern int value_in_thread_stack_temporaries (struct value *, ptid_t);
+
 extern struct thread_info *thread_list;
 
 #endif /* GDBTHREAD_H */
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 8e44b7c..1334e7e 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 14a1f08..a56f543 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1823,6 +1823,8 @@ extern int is_integral_type (struct type *);
 
 extern int is_scalar_type_recursive (struct type *);
 
+extern int class_or_union_p (const struct type *);
+
 extern void maintenance_print_type (char *, int);
 
 extern htab_t create_copied_types_hash (struct objfile *objfile);
diff --git a/gdb/infcall.c b/gdb/infcall.c
index bbac693..d7ab1f9 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.
+   If STACK_TEMPORARIES is non-zero, then the returned value will be on
+   lval_memory type and will be stored as a temporary on the thread's 
+   stack.  */
+
+static struct value *
+get_return_value_from_memory (struct type *type, CORE_ADDR addr,
+			      int stack_temporaries)
+{
+  struct value *retval;
+  if (stack_temporaries)
+    {
+      retval = value_from_contents_and_address (type, NULL, addr);
+      push_thread_stack_temporary (inferior_ptid, 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
@@ -495,6 +522,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
   ptid_t call_thread_ptid;
   struct gdb_exception e;
   char name_buf[RAW_FUNCTION_ADDRESS_SIZE];
+  int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid);
 
   if (TYPE_CODE (ftype) == TYPE_CODE_PTR)
     ftype = check_typedef (TYPE_TARGET_TYPE (ftype));
@@ -593,6 +621,16 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
          If the ABI specifies a "Red Zone" (see the doco) the code
          below will quietly trash it.  */
       sp = old_sp;
+
+    /* Skip over the stack temporaries that might have been generated during
+       the evaluation of an expression.  */
+    if (stack_temporaries)
+      {
+	if (gdbarch_inner_than (gdbarch, 1, 2))
+	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 1);
+	else
+	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 0);
+      }
   }
 
   funaddr = find_function_addr (function, &values_type);
@@ -719,9 +757,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
+      || (stack_temporaries && class_or_union_p (values_type)))
     {
       if (gdbarch_inner_than (gdbarch, 1, 2))
 	{
@@ -1059,31 +1109,27 @@ 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));
-    else if (TYPE_CODE (target_values_type) != TYPE_CODE_VOID)
+    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,
+					     stack_temporaries);
+    else
       {
-	/* If the function returns void, don't bother fetching the
-	   return value.  */
-	switch (gdbarch_return_value (gdbarch, function, target_values_type,
-				      NULL, NULL, NULL))
+	retval = allocate_value (values_type);
+	gdbarch_return_value (gdbarch, function, values_type,
+			      retbuf, value_contents_raw (retval), NULL);
+	if (stack_temporaries && class_or_union_p (values_type))
 	  {
-	  case RETURN_VALUE_REGISTER_CONVENTION:
-	  case RETURN_VALUE_ABI_RETURNS_ADDRESS:
-	  case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
-	    gdbarch_return_value (gdbarch, function, values_type,
-				  retbuf, value_contents_raw (retval), NULL);
-	    break;
-	  case RETURN_VALUE_STRUCT_CONVENTION:
-	    read_value_memory (retval, 0, 1, struct_addr,
-			       value_contents_raw (retval),
-			       TYPE_LENGTH (values_type));
-	    break;
+	    /* 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.  */
+	    value_force_lval (retval, struct_addr);
+	    push_thread_stack_temporary (inferior_ptid, retval);
 	  }
       }
 
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/thread.c b/gdb/thread.c
index 5c94264..4a7a26e 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -636,6 +636,131 @@ prune_threads (void)
     }
 }
 
+/* A simple struct to pass ptid data to a cleanup function.  */
+
+struct ptid_data
+{
+  ptid_t ptid;
+};
+
+/* Disable storing stack temporaries for the thread whose id is
+   stored in DATA.  */
+
+static void
+disable_thread_stack_temporaries (void *data)
+{
+  struct ptid_data *pd = data;
+  struct thread_info *tp = find_thread_ptid (pd->ptid);
+
+  if (tp != NULL)
+    tp->stack_temporaries_enabled = 0;
+
+  xfree (pd);
+}
+
+/* Enable storing stack temporaries for thread with id PTID and return a
+   cleanup which can disable and clear the stack temporaries.  */
+
+struct cleanup *
+enable_thread_stack_temporaries (ptid_t ptid)
+{
+  struct thread_info *tp = find_thread_ptid (ptid);
+  struct ptid_data *data;
+  struct cleanup *c;
+
+  gdb_assert (tp != NULL);
+
+  tp->stack_temporaries_enabled = 1;
+  tp->stack_temporaries = NULL;
+  data = (struct ptid_data *) xmalloc (sizeof (struct ptid_data));
+  data->ptid = ptid;
+  c = make_cleanup (disable_thread_stack_temporaries, data);
+  make_cleanup (VEC_cleanup (value_ptr), &tp->stack_temporaries);
+
+  return c;
+}
+
+/* Return non-zero value if stack temporaies are enabled for the thread
+   with id PTID.  */
+
+int
+thread_stack_temporaries_enabled_p (ptid_t ptid)
+{
+  struct thread_info *tp = find_thread_ptid (ptid);
+
+  if (tp == NULL)
+    return 0;
+  else
+    return tp->stack_temporaries_enabled;
+}
+
+/* Push V on to the stack temporaries of the thread with id PTID.  */
+
+void
+push_thread_stack_temporary (ptid_t ptid, struct value *v)
+{
+  struct thread_info *tp = find_thread_ptid (ptid);
+
+  gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
+  VEC_safe_push (value_ptr, tp->stack_temporaries, v);
+}
+
+/* Return 1 if VAL is among the stack temporaries of the thread
+   with id PTID.  Return 0 otherwise.  */
+   
+int
+value_in_thread_stack_temporaries (struct value *val, ptid_t ptid)
+{
+  struct thread_info *tp = find_thread_ptid (ptid);
+
+  gdb_assert (tp != NULL && tp->stack_temporaries_enabled);
+  if (!VEC_empty (value_ptr, tp->stack_temporaries))
+    {
+      struct value *v;
+      int i;
+
+      for (i = 0; VEC_iterate (value_ptr, tp->stack_temporaries, i, v); i++)
+	if (v == val)
+	  return 1;
+    }
+
+  return 0;
+}
+
+/* Return an address after skipping over the current stack temporaries
+   of thread with id PTID.  SP is the current stack frame pointer.  Non-zero
+   DOWNWARD indicates that the stack grows downwards/backwards.  */
+
+CORE_ADDR
+skip_thread_stack_temporaries (ptid_t ptid, CORE_ADDR sp, int downward)
+{
+  CORE_ADDR addr = sp;
+  struct thread_info *tp = find_thread_ptid (ptid);
+
+  gdb_assert (tp != NULL);
+  if (!VEC_empty (value_ptr, tp->stack_temporaries))
+    {
+      struct value *v = VEC_last (value_ptr, tp->stack_temporaries);
+      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;
+}
+
 void
 thread_change_ptid (ptid_t old_ptid, ptid_t new_ptid)
 {
diff --git a/gdb/value.c b/gdb/value.c
index ecfb154..b02713d 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -1724,6 +1724,18 @@ value_non_lval (struct value *arg)
    return arg;
 }
 
+/* Write contents of V at ADDR and set its lval type to be LVAL_MEMORY.  */
+
+void 
+value_force_lval (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)));
+  v->lval = lval_memory;
+  v->location.address = addr;
+}
+
 void
 set_value_component_location (struct value *component,
 			      const struct value *whole)
diff --git a/gdb/value.h b/gdb/value.h
index e3603c3..a8c33fb 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1038,6 +1038,8 @@ extern struct value *value_copy (struct value *);
 
 extern struct value *value_non_lval (struct value *);
 
+extern void value_force_lval (struct value *, CORE_ADDR);
+
 extern void preserve_one_value (struct value *, struct objfile *, htab_t);
 
 /* From valops.c */

Re: [PATCH v5] Make chained function calls in expressions work

[Siva Chandra]

On Sun, Nov 16, 2014 at 10:08 PM, Siva Chandra <sivachandra@google.com> wrote:
> Link to v4: https://sourceware.org/ml/gdb-patches/2014-10/msg00697.html
>
> I have made all the changes suggested by Ulrich Weigand in his comments for v4.
>
> gdb/ChangeLog:
>
> 2014-11-16  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * eval.c: Include gdbthread.h.
>         (evaluate_subexp): Enable thread stack temporaries before
>         evaluating a complete expression and clean them up after the
>         evaluation is complete.
>         * gdbthread.h: Include common/vec.h.
>         (value_ptr): New typedef.
>         (VEC (value_ptr)): New vector type.
>         (value_vec): New typedef.
>         (struct thread_info): Add new fields stack_temporaries_enabled
>         and stack_temporaries.
>         (enable_thread_stack_temporaries)
>         (thread_stack_temporaries_enabled_p, push_thread_stack_temporary)
>         (skip_thread_stack_temporaries)
>         (value_in_thread_stack_temporaries): Declare.
>         * gdbtypes.c (class_or_union_p): New function.
>         * gdbtypes.h (class_or_union_p): Declare.
>         * infcall.c (get_return_value_from_memory): New function.
>         (call_function_by_hand): Store return values of class type as
>         temporaries on stack.
>         * thread.c (enable_thread_stack_temporaries): New function.
>         (thread_stack_temporaries_enabled_p, push_thread_stack_temporary)
>         (skip_thread_stack_temporaries): Likewise.
>         (value_in_thread_stack_temporaries): Likewise.
>         (struct ptid_data): New type.
>         * value.c (value_force_lval): New function.
>         * value.h (value_force_lval): Declare.
>
> gdb/testsuite/ChangeLog:
>
> 2014-11-16  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 for "p c2->inta".

Ping.

Re: [PATCH v5] Make chained function calls in expressions work

[Ulrich Weigand]

Siva Chandra wrote:

> Link to v4: https://sourceware.org/ml/gdb-patches/2014-10/msg00697.html
> 
> I have made all the changes suggested by Ulrich Weigand in his comments for v4.

Thanks!  It looks nearly ready to commit now.  I still have a couple of
comments, but they're mostly cosmetic at this point ...  (Except for the
stack alignment issue, which is an actual bug.)

>  evaluate_subexp (struct type *expect_type, struct expression *exp,
>  		 int *pos, enum noside noside)
>  {
> -  return (*exp->language_defn->la_exp_desc->evaluate_exp) 
> +  struct cleanup *cleanups;
> +  struct value *retval;
> +  int cleanup_temps = 0;
> +
> +  if (*pos == 0 && inferior_ptid.pid != 0

We should avoid directly accessing subfields of ptid_t.  What is the
purpose of the .pid != 0 test here?  Is this simply to check whether
we can actually execute inferior calls?  In that case, it would be
preferable to check "target_has_execution", just as is done in
call_function_by_hand.  (This should end up doing basically the same
test, but properly abstracted.)

> +      && exp->language_defn->la_language == language_cplus)

I like the language check, this addresses one of my concerns about not
adding extra overhead when debugging plain C.  In the future, certain
other languages (Ada?) might also want to take advantage of the stack
temporary feature, in which case we might want to make this a property
of the language vector, instead of the explicit language_cplus check.
But that can wait until we actually have a second instance ...

> +/* 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.
> +   If STACK_TEMPORARIES is non-zero, then the returned value will be on
> +   lval_memory type and will be stored as a temporary on the thread's 
> +   stack.  */
> +
> +static struct value *
> +get_return_value_from_memory (struct type *type, CORE_ADDR addr,
> +			      int stack_temporaries)
> +{
> +  struct value *retval;
> +  if (stack_temporaries)
> +    {
> +      retval = value_from_contents_and_address (type, NULL, addr);
> +      push_thread_stack_temporary (inferior_ptid, retval);
> +    }
> +  else
> +    {
> +      retval = allocate_value (type);
> +      read_value_memory (retval, 0, 1, addr, value_contents_raw (retval),
> +			 TYPE_LENGTH (type));
> +    }
> +
> +  return retval;
> +}

Due the the restructuring of the return value handling logic you implemented
in the patch, there is now only a single use of this routine.  Maybe it would
make sense to just inline the code at the call site to keep the overall logic
closer together  ...

> +    /* Skip over the stack temporaries that might have been generated during
> +       the evaluation of an expression.  */
> +    if (stack_temporaries)
> +      {
> +	if (gdbarch_inner_than (gdbarch, 1, 2))
> +	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 1);
> +	else
> +	  sp = skip_thread_stack_temporaries (inferior_ptid, sp, 0);
> +      }

As mentioned in one of the previous review mails, you now need to verify
that the new SP is still correctly aligned (and re-align if necessary).

The split of responsibilities between infcall.c and thread.c seems a bit
suboptimal; the skip_thread_stack_temporaries routine makes a lot of
assumptions that are only true when called from just this particular place.

Maybe it would be better to have a function in thread.c that simply returns
a range (low .. high) of addresses spanned by temporaries in the current
thread, and then have infcall.c use this range to implement the skipping,
taking into account gdbarch_inner_than, alignment rules, etc.


> +/* A simple struct to pass ptid data to a cleanup function.  */
> +
> +struct ptid_data
> +{
> +  ptid_t ptid;
> +};

I don't understand why this struct is needed ...  Can't you just
allocate a ptid_t directly?  I.e. use something like:

  data = (ptid_t *) xmalloc (sizeof (ptid_t));
  *data = ptid;

> +/* Disable storing stack temporaries for the thread whose id is
> +   stored in DATA.  */
> +
> +static void
> +disable_thread_stack_temporaries (void *data)
> +{
> +  struct ptid_data *pd = data;
> +  struct thread_info *tp = find_thread_ptid (pd->ptid);
> +
> +  if (tp != NULL)
> +    tp->stack_temporaries_enabled = 0;
> +
> +  xfree (pd);

Why doesn't this also free the stack_temporaries vector itself?
This would save a second cleanup (and also make sense anyway).

> +/* Return non-zero value if stack temporaies are enabled for the thread

Typo.


Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com