[PATCH 4/4] m68k: Install return_with_first_hidden_param_p

[PATCH 4/4] m68k: Install return_with_first_hidden_param_p

[Yao Qi]

Build m68k-linux gdb.  Completely untested.

gdb:

2012-04-18  Yao Qi  <yao@codesourcery.com>

	* m68k-tdep.c (m68k_return_with_first_hidden_param_p): New.
	(m68k_gdbarch_init): Install m68k_return_with_first_hidden_param_p.
---
 gdb/m68k-tdep.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index ee72026..fe609c2 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -1051,6 +1051,16 @@ m68k_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 }
 \f
 
+/* This is the implementation of gdbarch method
+   return_with_first_hidden_param_p.  */
+
+static int
+m68k_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
+				       struct type *type)
+{
+  return 0;
+}
+
 /* System V Release 4 (SVR4).  */
 
 void
@@ -1236,6 +1246,8 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Function call & return.  */
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
+  set_gdbarch_return_with_first_hidden_param_p (gdbarch,
+						m68k_return_with_first_hidden_param_p);
 
 
   /* Disassembler.  */
-- 
1.7.0.4

[PATCH 2/4] tic6x: Install return_with_first_hidden_param_p

[Yao Qi]

This patch can be logically separated to two parts,
 - 1) Remove local variable `cplus_return_struct_by_reference' in
tic6x_push_dummy_call
 - 2) Implement hook return_with_first_hidden_param_p.

As we can see in the comment to `cplus_return_struct_by_reference',
it is used to address the same problem as what gdbarch hook
`return_with_first_hidden_param_p' tries to address.  Originally,
I used a tic6x-specific approach (`cplus_return_struct_by_reference')
to address this issue, because at that moment, I thought that
problem is tic6x specific.  Today, we realize that this kind
of problem exists on targets more than tic6x, such as SH, m68k, etc.

This patch is to remove code of using `cplus_return_struct_by_reference',
and switch to use this new gdbarch hook.

Regression tested for tic6x-uclinux.

gdb:

2012-04-16  Yao Qi  <yao@codesourcery.com>

	* tic6x-tdep.c (tic6x_push_dummy_call): Remove local variable
	`cplus_return_struct_by_reference'.
	(tic6x_return_value): Handle language cplusplus.
	(tic6x_return_with_first_hidden_param_p): New.
	(tic6x_gdbarch_init): Install tic6x_return_with_first_hidden_param_p.
---
 gdb/tic6x-tdep.c |   65 ++++++++++++++++++++++++-----------------------------
 1 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
index 2dce8da..815522f 100644
--- a/gdb/tic6x-tdep.c
+++ b/gdb/tic6x-tdep.c
@@ -825,6 +825,19 @@ tic6x_return_value (struct gdbarch *gdbarch, struct type *func_type,
 		    struct type *type, struct regcache *regcache,
 		    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
+  /* In C++, when function returns an object, even its size is small
+     enough, it stii has to be passed via reference, pointed by register
+     A3.  */
+  if (current_language->la_language == language_cplus)
+    {
+      if (type != NULL)
+	{
+	  CHECK_TYPEDEF (type);
+	  if (language_pass_by_reference (type))
+	    return RETURN_VALUE_STRUCT_CONVENTION;
+	}
+    }
+
   if (TYPE_LENGTH (type) > 8)
     return RETURN_VALUE_STRUCT_CONVENTION;
 
@@ -915,32 +928,7 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   /* The first arg passed on stack.  Mostly the first 10 args are passed by
      registers.  */
   int first_arg_on_stack = 10;
-  /* If this inf-call is a cpp method call, and return value is passed by
-     reference, this flag is set to 1, otherwise set to 0.  We need this flag
-     because computation of the return location in
-     infcall.c:call_function_by_hand is wrong for C6000 ELF ABI.  In
-     call_function_by_hand, the language is considered first, and then
-     target ABI is considered.  If language_pass_by_reference returns true,
-     the return location is passed as the first parameter to the function,
-     which is conflict with C6000 ELF ABI.  If this flag is true, we should
-     adjust args and return locations accordingly to comply with C6000 ELF
-     ABI.  */
-  int cplus_return_struct_by_reference = 0;
-
-  if (current_language->la_language == language_cplus)
-    {
-      struct type *values_type;
-
-      find_function_addr (function, &values_type);
-
-      if (values_type)
-	{
-	  CHECK_TYPEDEF (values_type);
-	  if (language_pass_by_reference (values_type))
-	    cplus_return_struct_by_reference = 1;
-	}
 
-    }
   /* Set the return address register to point to the entry point of
      the program, where a breakpoint lies in wait.  */
   regcache_cooked_write_unsigned (regcache, TIC6X_RA_REGNUM, bp_addr);
@@ -950,12 +938,6 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
      the address in A3.  */
   if (struct_return)
     regcache_cooked_write_unsigned (regcache, 3, struct_addr);
-  else if (cplus_return_struct_by_reference)
-    /* When cplus_return_struct_by_reference is 1, means local variable
-       lang_struct_return in call_function_by_hand is 1, so struct is
-       returned by reference, even STRUCT_RETURN is 0.  Note that STRUCT_ADDR
-       is still valid in this case.  */
-    regcache_cooked_write_unsigned (regcache, 3, struct_addr);
 
   /* Determine the type of this function.  */
   func_type = check_typedef (func_type);
@@ -970,10 +952,8 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   if (TYPE_VARARGS (func_type))
     first_arg_on_stack = TYPE_NFIELDS (func_type) - 1;
 
-  /* Now make space on the stack for the args.  If
-     cplus_return_struct_by_reference is 1, means GDB pass an extra parameter
-     in ARGS, which is useless here, skip it.  */
-  for (argnum = cplus_return_struct_by_reference; argnum < nargs; argnum++)
+  /* Now make space on the stack for the args.  */
+  for (argnum = 0; argnum < nargs; argnum++)
     {
       int len = align_up (TYPE_LENGTH (value_type (args[argnum])), 4);
       if (argnum >= 10 - argreg)
@@ -989,7 +969,7 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   /* Now load as many as possible of the first arguments into
      registers, and push the rest onto the stack.  Loop through args
      from first to last.  */
-  for (argnum = cplus_return_struct_by_reference; argnum < nargs; argnum++)
+  for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
       struct value *arg = args[argnum];
@@ -1210,6 +1190,16 @@ tic6x_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
   return 1;
 }
 
+/* This is the implementation of gdbarch method
+   return_with_first_hidden_param_p.  */
+
+static int
+tic6x_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
+					struct type *type)
+{
+  return 0;
+}
+
 static struct gdbarch *
 tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -1368,6 +1358,9 @@ tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_in_function_epilogue_p (gdbarch, tic6x_in_function_epilogue_p);
 
+  set_gdbarch_return_with_first_hidden_param_p (gdbarch,
+						tic6x_return_with_first_hidden_param_p);
+
   /* Hook in ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);
 
-- 
1.7.0.4

[PATCH 3/4] sh: Install return_with_first_hidden_param_p

[Yao Qi]

SH specific bits.

Regression tested on sh-linux-gnu.  Two fails are fixed

-FAIL: gdb.cp/bs15503.exp: print (const char *) s.substr(0,4)
-FAIL: gdb.cp/bs15503.exp: print (const char *) (s=s.substr(0,4))
+PASS: gdb.cp/bs15503.exp: print (const char *) s.substr(0,4)
+PASS: gdb.cp/bs15503.exp: print (const char *) (s=s.substr(0,4))

gdb:

2012-04-16  Yao Qi  <yao@codesourcery.com>

	* sh-tdep.c (sh_return_with_first_hidden_param_p): New.
	(sh_gdbarch_init): Install sh_return_with_first_hidden_param_p.
---
 gdb/sh-tdep.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index 7f15191..029958d 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -2915,6 +2915,17 @@ sh_regset_from_core_section (struct gdbarch *gdbarch, const char *sect_name,
 
   return NULL;
 }
+
+/* This is the implementation of gdbarch method
+   return_with_first_hidden_param_p.  */
+
+static int
+sh_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
+				     struct type *type)
+{
+  return 0;
+}
+
 \f
 
 static struct gdbarch *
@@ -3013,6 +3024,8 @@ sh_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
 
   set_gdbarch_push_dummy_call (gdbarch, sh_push_dummy_call_nofpu);
+  set_gdbarch_return_with_first_hidden_param_p (gdbarch,
+						sh_return_with_first_hidden_param_p);
 
   set_gdbarch_believe_pcc_promotion (gdbarch, 1);
 
-- 
1.7.0.4

[PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

Hi, this patch series is about fixing return values in C++ in GDB inf-call.

In C++ ABI, section 3.1.4:

"...if the return value type has a non-trivial copy constructor or
destructor, the caller allocates space for a temporary, and passes a
pointer to the temporary as an implicit first parameter preceding both
the this parameter and user parameters. The callee constructs the return
value into this temporary."

Looks GDB is correct on handling C++ inf-call of return value (by checking
language_pass_by_reference).  Unfortunately, GCC is not correct here.
currently GCC processes the return value RTL expanding like this, in
gcc/function.c:assign_parms_augmented_arg_list():

  /* If struct value address is treated as the first argument, make it
so.  */
  if (aggregate_value_p (DECL_RESULT (fndecl), fndecl)
      && ! cfun->returns_pcc_struct
      && targetm.calls.struct_value_rtx (TREE_TYPE (fndecl), 1) == 0)
    {
      tree type = build_pointer_type (TREE_TYPE (fntype));
      tree decl;
      ...

The use of the TARGET_STRUCT_VALUE_RTX hook to determine whether to
honor this convention, effectively makes what should be a
language-dependent rule, into a target-dependent convention instead.

The ia64 hook takes care to detect the C++ types to return NULL (which
is referred to in the comments as an ABI bug resolved in G++ 3.4).
Looking across other targets, sh, c6x, m68k, are examples of targets
which always return non-NULL in their TARGET_STRUCT_VALUE_RTX hooks, and
are exactly those that I also predicted and reproduced the same
symptoms.

C++ ABI is intended to be cross-architecture, but C++ ABI ends up being
target-dependent at some points.  In practice, GDB has to know which
architecture is special.

As I said above, GCC changes a language-dependent rule to a
target-dependent rule 'by mistake', so GDB has to be aware of this.  That
is the motivation to create this new gdbarch hook.  If the hook is not
installed, the default version still complies to existing GDB's behavior,
nothing is changed.

gdb:

2012-04-16  Yao Qi  <yao@codesourcery.com>
	    Chung-Lin Tang <cltang@codesourcery.com>

	* arch-utils.c (default_return_with_first_hidden_param_p): New.
	* arch-utils.h: Declare.
	* gdbarch.sh: Add return_with_first_hidden_param_p.
	* gdbarch.c, gdbarch.h: Regenerated.
	* infcall.c (call_function_by_hand): Call gdbarch_return_with_first_hidden_param_p
	instead of language_pass_by_reference.
---
 gdb/arch-utils.c |    8 ++++++++
 gdb/arch-utils.h |    2 ++
 gdb/gdbarch.c    |   24 ++++++++++++++++++++++++
 gdb/gdbarch.h    |    9 +++++++++
 gdb/gdbarch.sh   |    6 ++++++
 gdb/infcall.c    |   12 ++++++------
 6 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index fabb515..f68d211 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -31,6 +31,7 @@
 #include "osabi.h"
 #include "target-descriptions.h"
 #include "objfiles.h"
+#include "language.h"
 
 #include "version.h"
 
@@ -805,6 +806,13 @@ default_target_signal_from_host (struct gdbarch *gdbarch, int signo)
   return target_signal_from_host (signo);
 }
 
+int
+default_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
+					  struct type *type)
+{
+  return language_pass_by_reference (type);
+}
+
 /* */
 
 /* -Wmissing-prototypes */
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index c2c3398..15bd14d 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -177,4 +177,6 @@ extern enum target_signal default_target_signal_from_host (struct gdbarch *,
 extern int default_target_signal_to_host (struct gdbarch *,
 					  enum target_signal);
 
+extern int default_return_with_first_hidden_param_p (struct gdbarch *,
+						     struct type *);
 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index c079932..63ccecc 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -199,6 +199,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer;
   gdbarch_integer_to_address_ftype *integer_to_address;
   gdbarch_return_value_ftype *return_value;
+  gdbarch_return_with_first_hidden_param_p_ftype *return_with_first_hidden_param_p;
   gdbarch_skip_prologue_ftype *skip_prologue;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue;
   gdbarch_inner_than_ftype *inner_than;
@@ -357,6 +358,7 @@ struct gdbarch startup_gdbarch =
   unsigned_address_to_pointer,  /* address_to_pointer */
   0,  /* integer_to_address */
   0,  /* return_value */
+  default_return_with_first_hidden_param_p,  /* return_with_first_hidden_param_p */
   0,  /* skip_prologue */
   0,  /* skip_main_prologue */
   0,  /* inner_than */
@@ -498,6 +500,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->value_from_register = default_value_from_register;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
+  gdbarch->return_with_first_hidden_param_p = default_return_with_first_hidden_param_p;
   gdbarch->remote_breakpoint_from_pc = default_remote_breakpoint_from_pc;
   gdbarch->memory_insert_breakpoint = default_memory_insert_breakpoint;
   gdbarch->memory_remove_breakpoint = default_memory_remove_breakpoint;
@@ -645,6 +648,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
   /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of return_with_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
     fprintf_unfiltered (log, "\n\tskip_prologue");
   /* Skip verify of skip_main_prologue, has predicate.  */
@@ -1210,6 +1214,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: return_value = <%s>\n",
                       host_address_to_string (gdbarch->return_value));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: return_with_first_hidden_param_p = <%s>\n",
+                      host_address_to_string (gdbarch->return_with_first_hidden_param_p));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: sdb_reg_to_regnum = <%s>\n",
                       host_address_to_string (gdbarch->sdb_reg_to_regnum));
   fprintf_unfiltered (file,
@@ -2486,6 +2493,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+int
+gdbarch_return_with_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->return_with_first_hidden_param_p != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_return_with_first_hidden_param_p called\n");
+  return gdbarch->return_with_first_hidden_param_p (gdbarch, type);
+}
+
+void
+set_gdbarch_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
+                                              gdbarch_return_with_first_hidden_param_p_ftype return_with_first_hidden_param_p)
+{
+  gdbarch->return_with_first_hidden_param_p = return_with_first_hidden_param_p;
+}
+
 CORE_ADDR
 gdbarch_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR ip)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 84e6ff8..663251c 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -450,6 +450,15 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct type *functype, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return true if the return value of function is stored in the first hidden parameter.
+   In theory, this feature should be language-dependent, specified by language and its ABI,
+   such as C++.  Unfortunately, compiler may implement it to a target-dependent feature.
+   So that we need such hook here to be aware of this in GDB. */
+
+typedef int (gdbarch_return_with_first_hidden_param_p_ftype) (struct gdbarch *gdbarch, struct type *type);
+extern int gdbarch_return_with_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type);
+extern void set_gdbarch_return_with_first_hidden_param_p (struct gdbarch *gdbarch, gdbarch_return_with_first_hidden_param_p_ftype *return_with_first_hidden_param_p);
+
 typedef CORE_ADDR (gdbarch_skip_prologue_ftype) (struct gdbarch *gdbarch, CORE_ADDR ip);
 extern CORE_ADDR gdbarch_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR ip);
 extern void set_gdbarch_skip_prologue (struct gdbarch *gdbarch, gdbarch_skip_prologue_ftype *skip_prologue);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 5831172..ade7840 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -515,6 +515,12 @@ M:CORE_ADDR:integer_to_address:struct type *type, const gdb_byte *buf:type, buf
 # for instance).
 M:enum return_value_convention:return_value:struct type *functype, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf:functype, valtype, regcache, readbuf, writebuf
 
+# Return true if the return value of function is stored in the first hidden parameter.
+# In theory, this feature should be language-dependent, specified by language and its ABI,
+# such as C++.  Unfortunately, compiler may implement it to a target-dependent feature.
+# So that we need such hook here to be aware of this in GDB.
+m:int:return_with_first_hidden_param_p:struct type *type:type::default_return_with_first_hidden_param_p::0
+
 m:CORE_ADDR:skip_prologue:CORE_ADDR ip:ip:0:0
 M:CORE_ADDR:skip_main_prologue:CORE_ADDR ip:ip
 f:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs:0:0
diff --git a/gdb/infcall.c b/gdb/infcall.c
index 6c250e3..442965b 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -464,7 +464,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 {
   CORE_ADDR sp;
   struct type *values_type, *target_values_type;
-  unsigned char struct_return = 0, lang_struct_return = 0;
+  unsigned char struct_return = 0, hidden_first_param_p = 0;
   CORE_ADDR struct_addr = 0;
   struct infcall_control_state *inf_status;
   struct cleanup *inf_status_cleanup;
@@ -598,9 +598,9 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      the first argument is passed in out0 but the hidden structure
      return pointer would normally be passed in r8.  */
 
-  if (language_pass_by_reference (values_type))
+  if (gdbarch_return_with_first_hidden_param_p (gdbarch, values_type))
     {
-      lang_struct_return = 1;
+      hidden_first_param_p = 1;
 
       /* Tell the target specific argument pushing routine not to
 	 expect a value.  */
@@ -708,7 +708,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      stack, if necessary.  Make certain that the value is correctly
      aligned.  */
 
-  if (struct_return || lang_struct_return)
+  if (struct_return || hidden_first_param_p)
     {
       int len = TYPE_LENGTH (values_type);
 
@@ -734,7 +734,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	}
     }
 
-  if (lang_struct_return)
+  if (hidden_first_param_p)
     {
       struct value **new_args;
 
@@ -1040,7 +1040,7 @@ When the function is done executing, GDB will silently stop."),
     /* Figure out the value returned by the function.  */
     retval = allocate_value (values_type);
 
-    if (lang_struct_return)
+    if (hidden_first_param_p)
       read_value_memory (retval, 0, 1, struct_addr,
 			 value_contents_raw (retval),
 			 TYPE_LENGTH (values_type));
-- 
1.7.0.4

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 04/18/2012 09:17 PM, Yao Qi wrote:
> Hi, this patch series is about fixing return values in C++ in GDB inf-call.
> 
> In C++ ABI, section 3.1.4:
> 
> "...if the return value type has a non-trivial copy constructor or
> destructor, the caller allocates space for a temporary, and passes a
> pointer to the temporary as an implicit first parameter preceding both
> the this parameter and user parameters. The callee constructs the return
> value into this temporary."
> 
> Looks GDB is correct on handling C++ inf-call of return value (by checking
> language_pass_by_reference).  Unfortunately, GCC is not correct here.
> currently GCC processes the return value RTL expanding like this, in
> gcc/function.c:assign_parms_augmented_arg_list():
> 
>   /* If struct value address is treated as the first argument, make it
> so.  */
>   if (aggregate_value_p (DECL_RESULT (fndecl), fndecl)
>       && ! cfun->returns_pcc_struct
>       && targetm.calls.struct_value_rtx (TREE_TYPE (fndecl), 1) == 0)
>     {
>       tree type = build_pointer_type (TREE_TYPE (fntype));
>       tree decl;
>       ...
> 
> The use of the TARGET_STRUCT_VALUE_RTX hook to determine whether to
> honor this convention, effectively makes what should be a
> language-dependent rule, into a target-dependent convention instead.
> 
> The ia64 hook takes care to detect the C++ types to return NULL (which
> is referred to in the comments as an ABI bug resolved in G++ 3.4).
> Looking across other targets, sh, c6x, m68k, are examples of targets
> which always return non-NULL in their TARGET_STRUCT_VALUE_RTX hooks, and
> are exactly those that I also predicted and reproduced the same
> symptoms.
> 
> C++ ABI is intended to be cross-architecture, but C++ ABI ends up being
> target-dependent at some points.  In practice, GDB has to know which
> architecture is special.
> 
> As I said above, GCC changes a language-dependent rule to a
> target-dependent rule 'by mistake', so GDB has to be aware of this.  That
> is the motivation to create this new gdbarch hook.  If the hook is not
> installed, the default version still complies to existing GDB's behavior,
> nothing is changed.
> 
> gdb:
> 
> 2012-04-16  Yao Qi  <yao@codesourcery.com>
> 	    Chung-Lin Tang <cltang@codesourcery.com>
> 
> 	* arch-utils.c (default_return_with_first_hidden_param_p): New.
> 	* arch-utils.h: Declare.
> 	* gdbarch.sh: Add return_with_first_hidden_param_p.
> 	* gdbarch.c, gdbarch.h: Regenerated.
> 	* infcall.c (call_function_by_hand): Call gdbarch_return_with_first_hidden_param_p
> 	instead of language_pass_by_reference.

Ping.  http://sourceware.org/ml/gdb-patches/2012-04/msg00565.html

-- 
Yao (齐尧)

[ping 2] : [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 04/18/2012 09:17 PM, Yao Qi wrote:
> Hi, this patch series is about fixing return values in C++ in GDB inf-call.
> 
> In C++ ABI, section 3.1.4:
> 
> "...if the return value type has a non-trivial copy constructor or
> destructor, the caller allocates space for a temporary, and passes a
> pointer to the temporary as an implicit first parameter preceding both
> the this parameter and user parameters. The callee constructs the return
> value into this temporary."
> 
> Looks GDB is correct on handling C++ inf-call of return value (by checking
> language_pass_by_reference).  Unfortunately, GCC is not correct here.
> currently GCC processes the return value RTL expanding like this, in
> gcc/function.c:assign_parms_augmented_arg_list():
> 
>   /* If struct value address is treated as the first argument, make it
> so.  */
>   if (aggregate_value_p (DECL_RESULT (fndecl), fndecl)
>       && ! cfun->returns_pcc_struct
>       && targetm.calls.struct_value_rtx (TREE_TYPE (fndecl), 1) == 0)
>     {
>       tree type = build_pointer_type (TREE_TYPE (fntype));
>       tree decl;
>       ...
> 
> The use of the TARGET_STRUCT_VALUE_RTX hook to determine whether to
> honor this convention, effectively makes what should be a
> language-dependent rule, into a target-dependent convention instead.
> 
> The ia64 hook takes care to detect the C++ types to return NULL (which
> is referred to in the comments as an ABI bug resolved in G++ 3.4).
> Looking across other targets, sh, c6x, m68k, are examples of targets
> which always return non-NULL in their TARGET_STRUCT_VALUE_RTX hooks, and
> are exactly those that I also predicted and reproduced the same
> symptoms.
> 
> C++ ABI is intended to be cross-architecture, but C++ ABI ends up being
> target-dependent at some points.  In practice, GDB has to know which
> architecture is special.
> 
> As I said above, GCC changes a language-dependent rule to a
> target-dependent rule 'by mistake', so GDB has to be aware of this.  That
> is the motivation to create this new gdbarch hook.  If the hook is not
> installed, the default version still complies to existing GDB's behavior,
> nothing is changed.
> 
> gdb:
> 
> 2012-04-16  Yao Qi  <yao@codesourcery.com>
> 	    Chung-Lin Tang <cltang@codesourcery.com>
> 
> 	* arch-utils.c (default_return_with_first_hidden_param_p): New.
> 	* arch-utils.h: Declare.
> 	* gdbarch.sh: Add return_with_first_hidden_param_p.
> 	* gdbarch.c, gdbarch.h: Regenerated.
> 	* infcall.c (call_function_by_hand): Call gdbarch_return_with_first_hidden_param_p
> 	instead of language_pass_by_reference.

I read this patch roughly again just now, and don't see anything
obviously wrong.  Can they go in?

Ping 2.  http://sourceware.org/ml/gdb-patches/2012-04/msg00565.html

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> C++ ABI is intended to be cross-architecture, but C++ ABI ends up being
> target-dependent at some points.  In practice, GDB has to know which
> architecture is special.
> 
> As I said above, GCC changes a language-dependent rule to a
> target-dependent rule 'by mistake', so GDB has to be aware of this.  That
> is the motivation to create this new gdbarch hook.  If the hook is not
> installed, the default version still complies to existing GDB's behavior,
> nothing is changed.

I am of two minds about this. If this is a GCC error, then we should
also think of the error being fixed in GCC - at least at some point.
So, anything we end up doing here should be regarded as a workaround,
and I wouldn't want the workaround to start causing problems the day
the problem is fixed in GCC.

We have the same sort of issue in Ada, where functions returning
types whose size is non-static are transformed into procedures
where the first parameter is the return value. I haven't had the time
to look into this - in particular whether this feature is target-
dependent or not. I think we can get away with it by simply inspecting
the funtion's parameter name (which is not great).

It'd be good if you could show us the debugging information generated
for functions that return values vial the first parameter. Ideally,
there should be some information about this in the functions debug
info. I think that this would be the best way forward - that way,
GDB wouldn't have to juggle a number of factors in order to guess
which convention to use.

In terms of implementation, I think it would be better if we passed
the function's symbol as a parameter to the new gdbarch method, rather
than the return time.  With the symbol, we can determine the language,
which might be a useful thing to have when determining how the result
is returned.

Just a nit, I would name the new method "return_in_hidden_first_param_p"
(although, I think we have more general issues to discuss before we
can really talk about the code).


-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/03/2012 09:14 AM, Joel Brobecker wrote:
> I am of two minds about this. If this is a GCC error, then we should
> also think of the error being fixed in GCC - at least at some point.
> So, anything we end up doing here should be regarded as a workaround,
> and I wouldn't want the workaround to start causing problems the day
> the problem is fixed in GCC.
> 
Joel, thanks for the review.

I am not sure we can call this an "error" in GCC, especially it is used
for some years.  The ABI of different arch may overwrite some rules in
general ABI, like tic6x in this case.

> We have the same sort of issue in Ada, where functions returning
> types whose size is non-static are transformed into procedures
> where the first parameter is the return value. I haven't had the time
> to look into this - in particular whether this feature is target-
> dependent or not. I think we can get away with it by simply inspecting
> the funtion's parameter name (which is not great).
> 
> It'd be good if you could show us the debugging information generated
> for functions that return values vial the first parameter. Ideally,
> there should be some information about this in the functions debug
> info. I think that this would be the best way forward - that way,
> GDB wouldn't have to juggle a number of factors in order to guess
> which convention to use.

I don't know how debug info helps here.  In fact, the patch is more
about "argument passing" rather than "value return".  On some arch,
function "s.substr(0,4)" expects three parameters "this", "0", and "4",
while other some other arch, it expects four, "return location", "this",
"0", and "4".  However, GDB pass four parameters unconditionally, which
causes trouble.

The debug information (.debug_frame is most relevant in this case I
guess) is about in a certain function, which registers are stored in
which place.  So debug infor can't tell how registers are used to pass
parameters during a function call, which should be covered by "calling
convention", usually part of ABI.

> 
> In terms of implementation, I think it would be better if we passed
> the function's symbol as a parameter to the new gdbarch method, rather
> than the return time.  With the symbol, we can determine the language,
> which might be a useful thing to have when determining how the result
> is returned.
> 
> Just a nit, I would name the new method "return_in_hidden_first_param_p"
> (although, I think we have more general issues to discuss before we
> can really talk about the code).

The new hook name might be renamed to
'pass_return_loc_in_first_hidden_param_p'.

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Chung-Lin Tang]

On 12/5/3 9:14 AM, Joel Brobecker wrote:
>> C++ ABI is intended to be cross-architecture, but C++ ABI ends up being
>> target-dependent at some points.  In practice, GDB has to know which
>> architecture is special.
>>
>> As I said above, GCC changes a language-dependent rule to a
>> target-dependent rule 'by mistake', so GDB has to be aware of this.  That
>> is the motivation to create this new gdbarch hook.  If the hook is not
>> installed, the default version still complies to existing GDB's behavior,
>> nothing is changed.
> 
> I am of two minds about this. If this is a GCC error, then we should
> also think of the error being fixed in GCC - at least at some point.
> So, anything we end up doing here should be regarded as a workaround,
> and I wouldn't want the workaround to start causing problems the day
> the problem is fixed in GCC.

It's hard to say whether this should be something fixed in GCC at all,
for this just creates yet another C++ ABI version (at least for some
architectures) :P

> We have the same sort of issue in Ada, where functions returning
> types whose size is non-static are transformed into procedures
> where the first parameter is the return value. I haven't had the time
> to look into this - in particular whether this feature is target-
> dependent or not. I think we can get away with it by simply inspecting
> the funtion's parameter name (which is not great).
> 
> It'd be good if you could show us the debugging information generated
> for functions that return values vial the first parameter. Ideally,
> there should be some information about this in the functions debug
> info. I think that this would be the best way forward - that way,
> GDB wouldn't have to juggle a number of factors in order to guess
> which convention to use.

There isn't really anything wrong with how GDB determines this from the
type information (the gnu-v3-abi.c code here for C++). The problem is
that GCC is inaccurate, making this kind of language-dependent decision
impossible in general; it has to be augmented with target-specific
information.

> In terms of implementation, I think it would be better if we passed
> the function's symbol as a parameter to the new gdbarch method, rather
> than the return time.  With the symbol, we can determine the language,
> which might be a useful thing to have when determining how the result
> is returned.

Currently, as you can see from the small GCC code excerpt from Yao's
first mail, GCC internally abstracts away the details of 'aggregate
types', so I'm not sure if there are any language specific
considerations here...

If convenient, maybe you can try out how the manual call feature for Ada
works? i.e. call a function where the aforementioned kind of Ada type is
returned, and see how GDB behaves. If SH or C6x is too exotic, hppa and
(probably) 32-bit SPARC should also exhibit similarly failing behavior.

Thanks,
Chung-Lin

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> I am not sure we can call this an "error" in GCC, especially it is used
> for some years.  The ABI of different arch may overwrite some rules in
> general ABI, like tic6x in this case.

I guess that depends, but let's not argue over this.

> I don't know how debug info helps here.  In fact, the patch is more
> about "argument passing" rather than "value return".  On some arch,
> function "s.substr(0,4)" expects three parameters "this", "0", and "4",
> while other some other arch, it expects four, "return location", "this",
> "0", and "4".

I'd like to know what the code is, and what the associated debug info
looks like today. How many parameters would the debug info list for
your function above when the return value is passed as a hidden
parameter?  I am guessing 4. I am pretty sure that this is what GNAT
does for Ada (IIRC, GNAT emits a parameter named "TARGET" for it).

What debug info could do for us is tag that special parameter
using an attribute like DW_AT_return_address_as_implicit_param.
Then we could do have a generic mechanism in GDB that picks it
up, and calls the function appropriately without having to have
insider knowledge as to when such parameter is being used.

I think there is one avenue that needs to be looked at with
respect to debugging info: Maybe consult with the DWARF committee
to see how this type of situation is supposed to be handled.
Perhaps it's already covered?

Now, I realize that getting the GCC guys to improve the debug
info might not be easily doable. Perhaps a possible option
is to identify the special hidden field using some heuristics
(field name, for instance), and then pretend that this parameter
had the special DWARF attribute. That way, the day GCC starts
describing the implict parameters better, it'll just work out
of the box, and we phase out the hack a few years later.

I should add that I am badly overloaded these days and I don't
see it getting better any time soon. It's possible that I am not
getting what the real problem is simply because I don't know C++
(I learnt the basics over 15 years ago and haven't used it since).
Anyone knowing C++ better should feel free to take the discussion
over.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/05/2012 01:58 AM, Joel Brobecker wrote:
> I'd like to know what the code is, and what the associated debug info
> looks like today. How many parameters would the debug info list for
> your function above when the return value is passed as a hidden
> parameter?  I am guessing 4. I am pretty sure that this is what GNAT
> does for Ada (IIRC, GNAT emits a parameter named "TARGET" for it).
> 

It is 3, unfortunately.  I got the similar debug info output on both x86
and tic6x:

 <2><233a>: Abbrev Number: 45 (DW_TAG_subprogram)
    <233b>   DW_AT_external    : 1
    <233c>   DW_AT_name        : (indirect string, offset: 0x1759):
substr
    <2340>   DW_AT_decl_file   : 9
    <2341>   DW_AT_decl_line   : 2004
    <2343>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x297d):
_ZNKSs6substrEjj
    <2347>   DW_AT_type        : <0x1160>
    <234b>   DW_AT_declaration : 1
    <234c>   DW_AT_sibling     : <0x2361>
 <3><2350>: Abbrev Number: 15 (DW_TAG_formal_parameter)
    <2351>   DW_AT_type        : <0x2466>
    <2355>   DW_AT_artificial  : 1
 <3><2356>: Abbrev Number: 16 (DW_TAG_formal_parameter)
    <2357>   DW_AT_type        : <0x3b>
 <3><235b>: Abbrev Number: 16 (DW_TAG_formal_parameter)
    <235c>   DW_AT_type        : <0x3b>

> What debug info could do for us is tag that special parameter
> using an attribute like DW_AT_return_address_as_implicit_param.
> Then we could do have a generic mechanism in GDB that picks it
> up, and calls the function appropriately without having to have
> insider knowledge as to when such parameter is being used.
> 

Yes, we need some new tag to describe for this situation.

> I think there is one avenue that needs to be looked at with
> respect to debugging info: Maybe consult with the DWARF committee
> to see how this type of situation is supposed to be handled.
> Perhaps it's already covered?

Yes, it is a good idea to ask in dwarf-discuss@ to see if people there
have some smart ideas.  Will do.

> 
> Now, I realize that getting the GCC guys to improve the debug
> info might not be easily doable. Perhaps a possible option
> is to identify the special hidden field using some heuristics
> (field name, for instance), and then pretend that this parameter
> had the special DWARF attribute. That way, the day GCC starts
> describing the implict parameters better, it'll just work out
> of the box, and we phase out the hack a few years later.
> 

The heuristics in my mind is to do prologue analysis, to see how many
parameters are expected in sub routine, but not sure how reliable and
effective it is.

> I should add that I am badly overloaded these days and I don't
> see it getting better any time soon. It's possible that I am not
> getting what the real problem is simply because I don't know C++
> (I learnt the basics over 15 years ago and haven't used it since).
> Anyone knowing C++ better should feel free to take the discussion
> over.

This problem is caused by C++ argument passing in inf-call, but not a
C++ problem, IMO.  Any one here should be able to comment on this patch,
don't be scared by C++ :)

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

Hi Yao,

>  <2><233a>: Abbrev Number: 45 (DW_TAG_subprogram)
>     <233b>   DW_AT_external    : 1
>     <233c>   DW_AT_name        : (indirect string, offset: 0x1759):
> substr
>     <2340>   DW_AT_decl_file   : 9
>     <2341>   DW_AT_decl_line   : 2004
>     <2343>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x297d):
> _ZNKSs6substrEjj
>     <2347>   DW_AT_type        : <0x1160>
>     <234b>   DW_AT_declaration : 1
>     <234c>   DW_AT_sibling     : <0x2361>
>  <3><2350>: Abbrev Number: 15 (DW_TAG_formal_parameter)
>     <2351>   DW_AT_type        : <0x2466>
>     <2355>   DW_AT_artificial  : 1


What's the relationship between the first parameter's type (DIE 0x2466)
and the function's published returned type (DIE 0x2466)? Is one a
reference type of the other, for instance?

Basically, what I'm trying to help us find is a heuristic that would
be based purely on the DWARF info, rather than on implementation
knowledge from the compiler.  That way, we can try supporting the
current compiler, while at least trying to define what the standard
method should be (meaning that you don't need to be the one who
coordinates the effort of cleaning that up).

> Yes, we need some new tag to describe for this situation.

I saw the discussion you started on dwarf-discuss. Thanks!

> The heuristics in my mind is to do prologue analysis, to see how many
> parameters are expected in sub routine, but not sure how reliable and
> effective it is.

I don't think that's a good idea. Too specialized, and too fragile.
And thanks to DWARF frame information, it's something we're slowly
getting away from.

> This problem is caused by C++ argument passing in inf-call, but not a
> C++ problem, IMO.  Any one here should be able to comment on this
> patch, don't be scared by C++ :)

I have a better picture of the problem thanks you the copy of
the debug info. I agree it's not a C++-specific problem. I was just
invinting others to comment as well. I don't want to be too lax
and allow a detection method which I think isn't going to scale well.
But at the same time, I don't want to be too strict, and discourage
you. Having others comment on the issue might bring some ideas we
haven't had yet, or correct me on my understanding.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/08/2012 04:13 AM, Joel Brobecker wrote:
> Hi Yao,
> 
>>  <2><233a>: Abbrev Number: 45 (DW_TAG_subprogram)
>>     <233b>   DW_AT_external    : 1
>>     <233c>   DW_AT_name        : (indirect string, offset: 0x1759):
>> substr
>>     <2340>   DW_AT_decl_file   : 9
>>     <2341>   DW_AT_decl_line   : 2004
>>     <2343>   DW_AT_MIPS_linkage_name: (indirect string, offset: 0x297d):
>> _ZNKSs6substrEjj
>>     <2347>   DW_AT_type        : <0x1160>
>>     <234b>   DW_AT_declaration : 1
>>     <234c>   DW_AT_sibling     : <0x2361>
>>  <3><2350>: Abbrev Number: 15 (DW_TAG_formal_parameter)
>>     <2351>   DW_AT_type        : <0x2466>
>>     <2355>   DW_AT_artificial  : 1
> 
> 
> What's the relationship between the first parameter's type (DIE 0x2466)
> and the function's published returned type (DIE 0x2466)? Is one a
                                              ^^^^^^^^^^^
I guess you meant DIE 0x1160 here.

> reference type of the other, for instance?

Yes, I think so.

 <2><22b>: Abbrev Number: 10 (DW_TAG_structure_type)
    <22c>   DW_AT_name        : (indirect string, offset: 0x17c): basic_string<char, std::char_traits<char>, std::allocator<char> >
    <230>   DW_AT_declaration : 1
    <231>   DW_AT_sibling     : <0x271>

 <1><1160>: Abbrev Number: 53 (DW_TAG_structure_type)
    ^^^^^^^  <- function's return type

    <1161>   DW_AT_specification: <0x22b>
    <1165>   DW_AT_byte_size   : 4
    <1166>   DW_AT_decl_file   : 7
    <1167>   DW_AT_decl_line   : 52
    <1168>   DW_AT_sibling     : <0x2461>

 <1><2466>: Abbrev Number: 38 (DW_TAG_pointer_type)
    ^^^^^^  <- 1st param's type

    <2467>   DW_AT_byte_size   : 4     
    <2468>   DW_AT_type        : <0x246c>
 <1><246c>: Abbrev Number: 37 (DW_TAG_const_type)
    <246d>   DW_AT_type        : <0x1160>     
                                 ^^^^^^^^

> Basically, what I'm trying to help us find is a heuristic that would
> be based purely on the DWARF info, rather than on implementation
> knowledge from the compiler.  That way, we can try supporting the
> current compiler, while at least trying to define what the standard
> method should be (meaning that you don't need to be the one who
> coordinates the effort of cleaning that up).

Agreed.  I stared at these debug info for a while, but unable to have
a clue on heuristics.  My feeling is that these debug info doesn't give
us more than what source code can give, but the heuristics we are looking
for are about the difference on different targets, given the same source.
That is to say, on different targets, although the number of parameters
is different, the debug info is almost the same and hard to get heuristics,

>> This problem is caused by C++ argument passing in inf-call, but not a
>> C++ problem, IMO.  Any one here should be able to comment on this
>> patch, don't be scared by C++ :)
> 
> I have a better picture of the problem thanks you the copy of
> the debug info. I agree it's not a C++-specific problem. I was just
> invinting others to comment as well. I don't want to be too lax
> and allow a detection method which I think isn't going to scale well.
> But at the same time, I don't want to be too strict, and discourage
> you. Having others comment on the issue might bring some ideas we
> haven't had yet, or correct me on my understanding.

Yeah, indeed! :)

-- 
Yao (齐尧) 

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> Agreed.  I stared at these debug info for a while, but unable to have
> a clue on heuristics.  My feeling is that these debug info doesn't give
> us more than what source code can give, but the heuristics we are looking
> for are about the difference on different targets, given the same source.
> That is to say, on different targets, although the number of parameters
> is different, the debug info is almost the same and hard to get heuristics,

Can we use the fact that functions that have their first parameter
be a nameless, artificial, paramter whose type is a pointer to
the return type? Or would this heuristics trigger in other situations?

If that doesn't work, then I am running out of ideas, and then we'll
have to start looking giving GDB insider knowledge. But I really don't
like that idea.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/11/2012 05:20 AM, Joel Brobecker wrote:
>> > Agreed.  I stared at these debug info for a while, but unable to have
>> > a clue on heuristics.  My feeling is that these debug info doesn't give
>> > us more than what source code can give, but the heuristics we are looking
>> > for are about the difference on different targets, given the same source.
>> > That is to say, on different targets, although the number of parameters
>> > is different, the debug info is almost the same and hard to get heuristics,
> Can we use the fact that functions that have their first parameter
> be a nameless, artificial, paramter whose type is a pointer to
> the return type? Or would this heuristics trigger in other situations?
> 

Yeah, I thought of that heuristics, but it still can't differentiate
passing 3 or 4 parameters.

So far, the discussion in dwarf-discuss@ gives me a feeling that
"DW_AT_artificial_type"/"DW_ART_return_block" looks quite promising for
this problem, but it requires changes in gcc/binutils, and I haven't
investigate the history of it to see why it is ada-specific, instead of
a general attribute.

> If that doesn't work, then I am running out of ideas, and then we'll
> have to start looking giving GDB insider knowledge. But I really don't
> like that idea.

Well, we have to compromise :)  Could you please re-consider this patch
series, as it is a trade-off, and not perfect?

If this patch series is still not acceptable, we may create a variant
(or sub-class) of gnu-v3-abi, say "gnu-v3-pass-implicit-param" or
something similar, for targets affected by this problem, and
`set_cp_abi_as_auto_default ("gnu-v3-pass-implicit-param")' in each
target backend.  (This is my rough thought, and need some experiments to
see it works or not).  Even, if GCC in the future starts to pass
implicit parameter for these ports, we can switch to gnu-v3, and users
just have to type `set cp-abi gnu-v3' without rebuilding their GDB.

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> > Can we use the fact that functions that have their first parameter
> > be a nameless, artificial, paramter whose type is a pointer to
> > the return type? Or would this heuristics trigger in other situations?
> 
> Yeah, I thought of that heuristics, but it still can't differentiate
> passing 3 or 4 parameters.

So, you are saying that we have functions who have an extra hidden
parameter used for something other than the function's return value?

> Well, we have to compromise :)  Could you please re-consider this patch
> series, as it is a trade-off, and not perfect?
> 
> If this patch series is still not acceptable, we may create a variant
> (or sub-class) of gnu-v3-abi, say "gnu-v3-pass-implicit-param" or
> something similar, for targets affected by this problem, and
> `set_cp_abi_as_auto_default ("gnu-v3-pass-implicit-param")' in each
> target backend.  (This is my rough thought, and need some experiments to
> see it works or not).  Even, if GCC in the future starts to pass
> implicit parameter for these ports, we can switch to gnu-v3, and users
> just have to type `set cp-abi gnu-v3' without rebuilding their GDB.

I'm not very familiar with the gnu-v3-abi... If we've explored
all avenues with trying to use the debug info, I'm satisfied with
the next best thing, which could possibly be your initial set of
patches.  I'd like someone else to be involved in the review,
however, just to get someone else's opinion.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/15/2012 01:14 AM, Joel Brobecker wrote:
>>> > > Can we use the fact that functions that have their first parameter
>>> > > be a nameless, artificial, paramter whose type is a pointer to
>>> > > the return type? Or would this heuristics trigger in other situations?
>> > 
>> > Yeah, I thought of that heuristics, but it still can't differentiate
>> > passing 3 or 4 parameters.
> So, you are saying that we have functions who have an extra hidden
> parameter used for something other than the function's return value?
> 

No, the extra hidden parameter's type is the reference of the function's
return type.  My point is if we only examine debug info, we don't know
whether hidden parameter is passed, because the debug info is the same
regardless of hidden parameter is passed or not.

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> > So, you are saying that we have functions who have an extra hidden
> > parameter used for something other than the function's return value?
> > 
> 
> No, the extra hidden parameter's type is the reference of the function's
> return type.  My point is if we only examine debug info, we don't know
> whether hidden parameter is passed, because the debug info is the same
> regardless of hidden parameter is passed or not.

I may be too naive, or maybe I am not understanding what you are saying,
but I find it horrifying that the compiler would generate a parameter
DIE for a function if that parameter is not actually passed when calling
that function. Did I misunderstand?

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Thomas Schwinge]

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

Hi!

On Thu, 10 May 2012 14:20:54 -0700, Joel Brobecker <brobecker@adacore.com> wrote:
> > Agreed.  I stared at these debug info for a while, but unable to have
> > a clue on heuristics.  My feeling is that these debug info doesn't give
> > us more than what source code can give, but the heuristics we are looking
> > for are about the difference on different targets, given the same source.
> > That is to say, on different targets, although the number of parameters
> > is different, the debug info is almost the same and hard to get heuristics,
> 
> Can we use the fact that functions that have their first parameter
> be a nameless, artificial, paramter whose type is a pointer to
> the return type? Or would this heuristics trigger in other situations?

Just a quick thought without having a lot of context on this issue --
what about the case where there isn't even any debug information
available for the function to be called?  Does this rule out the DWARF
route, or isn't that something to be prepared for?

> If that doesn't work, then I am running out of ideas, and then we'll
> have to start looking giving GDB insider knowledge. But I really don't
> like that idea.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Mark Kettenis]

> Date: Tue, 15 May 2012 14:50:58 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 05/15/2012 01:14 AM, Joel Brobecker wrote:
> >>> > > Can we use the fact that functions that have their first parameter
> >>> > > be a nameless, artificial, paramter whose type is a pointer to
> >>> > > the return type? Or would this heuristics trigger in other situations?
> >> > 
> >> > Yeah, I thought of that heuristics, but it still can't differentiate
> >> > passing 3 or 4 parameters.
> > So, you are saying that we have functions who have an extra hidden
> > parameter used for something other than the function's return value?
> > 
> 
> No, the extra hidden parameter's type is the reference of the function's
> return type.  My point is if we only examine debug info, we don't know
> whether hidden parameter is passed, because the debug info is the same
> regardless of hidden parameter is passed or not.

I'm still very confused.  This "hidden parameter" thing sounds exactly
like "struct_return".  So are you saying that on some architectures
gdbarch_return_value() return RETUN_VALUE_REGISTER_CONVENTION when if
fact for C++ (and not for plain C) it should return
RETURN_VALUE_STRUCT_CONVENTION?

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> Just a quick thought without having a lot of context on this issue --
> what about the case where there isn't even any debug information
> available for the function to be called?  Does this rule out the DWARF
> route, or isn't that something to be prepared for?

I am not actually entirely sure! This never really entered my mind.
But a quick look at the sources seems to suggest that call the
function with the arguments as provided...

Note that the Ada expression parser does some function resolution
based on parameter types, and could possibly narrow the matches
down to nothing. This part only deals with functions for which
we have debugging information, obviously, but does not preclude
calling a function without debug info. And looking at eval.c,
I think that we have the same sort of thing happening for C++
as well. Although, I suspect it's not so much about validation,
but rather figuring out the right way of calling methods.

Whether the debugger decides to check the debug info or not seems
to be left to the -tdep code; in the "push_dummy_call" gdbarch
routine, for instance. But even that doesn't validate the parameters,
I wouldn't think. A quick look at the amd64 version, for instance,
shows that the "function" parameter is not used.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/15/2012 11:01 PM, Joel Brobecker wrote:
>> > No, the extra hidden parameter's type is the reference of the function's
>> > return type.  My point is if we only examine debug info, we don't know
>> > whether hidden parameter is passed, because the debug info is the same
>> > regardless of hidden parameter is passed or not.
> I may be too naive, or maybe I am not understanding what you are saying,
> but I find it horrifying that the compiler would generate a parameter
> DIE for a function if that parameter is not actually passed when calling
> that function. Did I misunderstand?

The compiler doesn't generate the DIE for the hidden parameter (it is
not 'this', it is used to store the address of return value).  The
situation horrifying you doesn't exist.

My original words are "My point is if we only examine debug info, we
don't know whether hidden parameter is passed, because the debug info is
the same".  The "same" means compiler doesn't generate DIE for the
hidden parameter, so it is the same on the targets having hidden
parameter passed and the targets not having hidden parameter passed.

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/16/2012 02:02 AM, Mark Kettenis wrote:
>> > No, the extra hidden parameter's type is the reference of the function's
>> > return type.  My point is if we only examine debug info, we don't know
>> > whether hidden parameter is passed, because the debug info is the same
>> > regardless of hidden parameter is passed or not.
> I'm still very confused.  This "hidden parameter" thing sounds exactly
> like "struct_return".  So are you saying that on some architectures
> gdbarch_return_value() return RETUN_VALUE_REGISTER_CONVENTION when if
> fact for C++ (and not for plain C) it should return
> RETURN_VALUE_STRUCT_CONVENTION?

In an inferior call, there are two steps related to ABI, 1) passing
parameters, 2) fetching return value.  The "hidden parameter" is
relevant to both steps.  Your description above is correct, and it is
about step 2.  My patches are trying to fix the problems in step 1.  So
far, GDB passes hidden parameter for some language, but on some targets,
the hidden parameter is not passed by parameter register, it is passed
by other register specified by its ABI.  GDB is wrong if it still pass
hidden parameter to parameter register.

-- 
Yao (齐尧)

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Joel Brobecker]

> My original words are "My point is if we only examine debug info, we
> don't know whether hidden parameter is passed, because the debug info is
> the same".  The "same" means compiler doesn't generate DIE for the
> hidden parameter, so it is the same on the targets having hidden
> parameter passed and the targets not having hidden parameter passed.

I was confusing the nameless, artificial, DIE that's generated as one
of the parameters of the subroutine as the DIE associated to that
hidden parameters. What's that nameless+artificial parameter DIE, then?

Talking to Tom on IRC, he reminded me that this issue is something
ABI-related, and thus wouldn't show up in DWARF debug info. Your
last email seems to indicate that this would be correct.

Now, because I don't know the C++ ABI at all, I don't know whether
it is expected that some architectures would have a hidden parameter
for the return value, while some don't. Is that allowed? Regardless,
it sounds like a gdbarch method would the logical approach... If
that is the case, I'd rather someone with more knowledge with C++
did the actual review, because I wouldn't be able to say whether
the implementation makes sense or not.

-- 
Joel

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Hi, this patch series is about fixing return values in C++ in GDB inf-call.

Thanks.  I read through the whole thread and dug into the code a bit as
well.  I think your approach is fine.

Yao> Unfortunately, GCC is not correct here.  currently GCC processes
Yao> the return value RTL expanding like this, in
Yao> gcc/function.c:assign_parms_augmented_arg_list():

I suppose it would be nice to get some commitment on the GCC side to
declare this a non-bug, and part of the ABI.

Yao> +int
Yao> +default_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
Yao> +					  struct type *type)

This could use an introductory comment, just explaining it is an
implementation suitable for use by such-and-such gdbarch method.

Yao> +# Return true if the return value of function is stored in the first hidden parameter.
Yao> +# In theory, this feature should be language-dependent, specified by language and its ABI,
Yao> +# such as C++.  Unfortunately, compiler may implement it to a target-dependent feature.
Yao> +# So that we need such hook here to be aware of this in GDB.

Over-long lines.

Ok with those fixes.

Tom

Re: [PATCH 2/4] tic6x: Install return_with_first_hidden_param_p

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-04-16  Yao Qi  <yao@codesourcery.com>
Yao> 	* tic6x-tdep.c (tic6x_push_dummy_call): Remove local variable
Yao> 	`cplus_return_struct_by_reference'.
Yao> 	(tic6x_return_value): Handle language cplusplus.
Yao> 	(tic6x_return_with_first_hidden_param_p): New.
Yao> 	(tic6x_gdbarch_init): Install tic6x_return_with_first_hidden_param_p.

I think you should self-approve this one.
I did skim it and it seemed reasonable, but I really don't know anything
about this arch.

Tom

Re: [PATCH 3/4] sh: Install return_with_first_hidden_param_p

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 	* sh-tdep.c (sh_return_with_first_hidden_param_p): New.
Yao> 	(sh_gdbarch_init): Install sh_return_with_first_hidden_param_p.

Ok.

Tom

Re: [PATCH 4/4] m68k: Install return_with_first_hidden_param_p

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-04-18  Yao Qi  <yao@codesourcery.com>
Yao> 	* m68k-tdep.c (m68k_return_with_first_hidden_param_p): New.
Yao> 	(m68k_gdbarch_init): Install m68k_return_with_first_hidden_param_p.

Ok.

Tom

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> Now, because I don't know the C++ ABI at all, I don't know whether
Joel> it is expected that some architectures would have a hidden parameter
Joel> for the return value, while some don't. Is that allowed? Regardless,
Joel> it sounds like a gdbarch method would the logical approach... If
Joel> that is the case, I'd rather someone with more knowledge with C++
Joel> did the actual review, because I wouldn't be able to say whether
Joel> the implementation makes sense or not.

Basically what happened is that the ABI was intended to work one way for
C++, but then due to an oversight, some GCC targets applied the same
rule to all languages.  So, different arches have different ABIs in
practice.  This series, IIUC, just changes gdb to recognize the existing
reality.

Tom

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Mark Kettenis]

> From: Tom Tromey <tromey@redhat.com>
> Date: Wed, 16 May 2012 14:55:49 -0600
> 
> >>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> Hi, this patch series is about fixing return values in C++ in GDB inf-call.
> 
> Thanks.  I read through the whole thread and dug into the code a bit as
> well.  I think your approach is fine.
> 
> Yao> Unfortunately, GCC is not correct here.  currently GCC processes
> Yao> the return value RTL expanding like this, in
> Yao> gcc/function.c:assign_parms_augmented_arg_list():
> 
> I suppose it would be nice to get some commitment on the GCC side to
> declare this a non-bug, and part of the ABI.
> 
> Yao> +int
> Yao> +default_return_with_first_hidden_param_p (struct gdbarch *gdbarch,
> Yao> +					  struct type *type)
> 
> This could use an introductory comment, just explaining it is an
> implementation suitable for use by such-and-such gdbarch method.
> 
> Yao> +# Return true if the return value of function is stored in the first hidden parameter.
> Yao> +# In theory, this feature should be language-dependent, specified by language and its ABI,
> Yao> +# such as C++.  Unfortunately, compiler may implement it to a target-dependent feature.
> Yao> +# So that we need such hook here to be aware of this in GDB.
> 
> Over-long lines.
> 
> Ok with those fixes.

If this hasn't been committed yet, please wait.

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Mark Kettenis]

> Date: Wed, 16 May 2012 09:55:00 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 05/16/2012 02:02 AM, Mark Kettenis wrote:
> >> > No, the extra hidden parameter's type is the reference of the function's
> >> > return type.  My point is if we only examine debug info, we don't know
> >> > whether hidden parameter is passed, because the debug info is the same
> >> > regardless of hidden parameter is passed or not.
> > I'm still very confused.  This "hidden parameter" thing sounds exactly
> > like "struct_return".  So are you saying that on some architectures
> > gdbarch_return_value() return RETUN_VALUE_REGISTER_CONVENTION when if
> > fact for C++ (and not for plain C) it should return
> > RETURN_VALUE_STRUCT_CONVENTION?
> 
> In an inferior call, there are two steps related to ABI, 1) passing
> parameters, 2) fetching return value.  The "hidden parameter" is
> relevant to both steps.  Your description above is correct, and it is
> about step 2.

No, that's not quite correct.  push_dummy_call() gets passed a "struct
return" argument, so it is also about step 1.  Except that this
decision is unconditionally bypassed for GNU v3 C++ ABI because of the
language_pass_by_reference() check.

> My patches are trying to fix the problems in step 1.  So
> far, GDB passes hidden parameter for some language, but on some targets,
> the hidden parameter is not passed by parameter register, it is passed
> by other register specified by its ABI.  GDB is wrong if it still pass
> hidden parameter to parameter register.

But the proper place to take that decision is in
gnu-v3-abi.c:gnuv3_pass_by_reference(), since this problem is rather
specific to the C++ ABI used by GCC.  So I think you need a
gnuv3_pass_by_reference() gdbarch hook, and call that if it is it is
installed.  It's easy to do that, since get_type_arch() on the type
passed as an argument to gnuv3_pass_by_reference() function will give
you the gdbarch.

I think that would be far less confusing.

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Yao Qi]

On 05/17/2012 07:03 AM, Mark Kettenis wrote:
>> > Ok with those fixes.
> If this hasn't been committed yet, please wait.

Mark, 
Haven't heard you on the V2 of this patch series,

  [V2 PATCH 0/5] Handle first hidden parameter
  http://sourceware.org/ml/gdb-patches/2012-05/msg00885.html

Committed this version with the fixes to address Tom's comments.
Regression tested on x86_64-linux.

http://sourceware.org/ml/gdb-cvs/2012-06/msg00063.html

-- 
Yao (齐尧) 

gdb:

2012-06-08  Yao Qi  <yao@codesourcery.com>
	    Chung-Lin Tang <cltang@codesourcery.com>

	* arch-utils.c (default_return_in_first_hidden_param_p): New.
	* arch-utils.h: Declare.
	* gdbarch.sh: Add return_in_first_hidden_param_p.
	* gdbarch.c, gdbarch.h: Regenerated.
	* infcall.c (call_function_by_hand): Call
	gdbarch_return_in_first_hidden_param_p instead of
	language_pass_by_reference.

	* m68k-tdep.c (m68k_return_in_first_hidden_param_p): New.
	(m68k_gdbarch_init): Install m68k_return_in_first_hidden_param_p.
	* sh-tdep.c (sh_return_in_first_hidden_param_p): New.
	(sh_gdbarch_init): Install sh_return_in_first_hidden_param_p.
	* tic6x-tdep.c (tic6x_push_dummy_call): Remove local variable
	`cplus_return_struct_by_reference'.
	(tic6x_return_value): Handle language cplusplus.
	(tic6x_return_in_first_hidden_param_p): New.
	(tic6x_gdbarch_init): Install tic6x_return_in_first_hidden_param_p.
---
 gdb/arch-utils.c |   11 +++++++++
 gdb/arch-utils.h |    2 +
 gdb/gdbarch.c    |   24 ++++++++++++++++++++
 gdb/gdbarch.h    |   10 ++++++++
 gdb/gdbarch.sh   |    7 +++++
 gdb/infcall.c    |   12 +++++-----
 gdb/m68k-tdep.c  |   12 ++++++++++
 gdb/sh-tdep.c    |   13 ++++++++++
 gdb/tic6x-tdep.c |   65 ++++++++++++++++++++++++-----------------------------
 9 files changed, 114 insertions(+), 42 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index e683a2d..f4414b9 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -31,6 +31,7 @@
 #include "osabi.h"
 #include "target-descriptions.h"
 #include "objfiles.h"
+#include "language.h"
 
 #include "version.h"
 
@@ -793,6 +794,16 @@ default_gen_return_address (struct gdbarch *gdbarch,
   error (_("This architecture has no method to collect a return address."));
 }
 
+int
+default_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
+					struct type *type)
+{
+  /* Usually, the return value's address is stored the in the "first hidden"
+     parameter if the return value should be passed by reference, as
+     specified in ABI.  */
+  return language_pass_by_reference (type);
+}
+
 /* */
 
 /* -Wmissing-prototypes */
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index 7c398b3..ef4cb26 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -172,4 +172,6 @@ extern void default_gen_return_address (struct gdbarch *gdbarch,
 extern const char *default_auto_charset (void);
 extern const char *default_auto_wide_charset (void);
 
+extern int default_return_in_first_hidden_param_p (struct gdbarch *,
+						   struct type *);
 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 4d46a5d..f3d81a1 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -200,6 +200,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer;
   gdbarch_integer_to_address_ftype *integer_to_address;
   gdbarch_return_value_ftype *return_value;
+  gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p;
   gdbarch_skip_prologue_ftype *skip_prologue;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue;
   gdbarch_inner_than_ftype *inner_than;
@@ -368,6 +369,7 @@ struct gdbarch startup_gdbarch =
   unsigned_address_to_pointer,  /* address_to_pointer */
   0,  /* integer_to_address */
   0,  /* return_value */
+  default_return_in_first_hidden_param_p,  /* return_in_first_hidden_param_p */
   0,  /* skip_prologue */
   0,  /* skip_main_prologue */
   0,  /* inner_than */
@@ -519,6 +521,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->value_from_register = default_value_from_register;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
+  gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
   gdbarch->remote_breakpoint_from_pc = default_remote_breakpoint_from_pc;
   gdbarch->memory_insert_breakpoint = default_memory_insert_breakpoint;
   gdbarch->memory_remove_breakpoint = default_memory_remove_breakpoint;
@@ -665,6 +668,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
   /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
     fprintf_unfiltered (log, "\n\tskip_prologue");
   /* Skip verify of skip_main_prologue, has predicate.  */
@@ -1243,6 +1247,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: remote_register_number = <%s>\n",
                       host_address_to_string (gdbarch->remote_register_number));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
+                      host_address_to_string (gdbarch->return_in_first_hidden_param_p));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_return_value_p() = %d\n",
                       gdbarch_return_value_p (gdbarch));
   fprintf_unfiltered (file,
@@ -2555,6 +2562,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+int
+gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->return_in_first_hidden_param_p != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_return_in_first_hidden_param_p called\n");
+  return gdbarch->return_in_first_hidden_param_p (gdbarch, type);
+}
+
+void
+set_gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
+                                            gdbarch_return_in_first_hidden_param_p_ftype return_in_first_hidden_param_p)
+{
+  gdbarch->return_in_first_hidden_param_p = return_in_first_hidden_param_p;
+}
+
 CORE_ADDR
 gdbarch_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR ip)
 {
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index a82e8bb..a239aa4 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -457,6 +457,16 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return true if the return value of function is stored in the first hidden
+   parameter.  In theory, this feature should be language-dependent, specified
+   by language and its ABI, such as C++.  Unfortunately, compiler may
+   implement it to a target-dependent feature.  So that we need such hook here
+   to be aware of this in GDB. */
+
+typedef int (gdbarch_return_in_first_hidden_param_p_ftype) (struct gdbarch *gdbarch, struct type *type);
+extern int gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type);
+extern void set_gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p);
+
 typedef CORE_ADDR (gdbarch_skip_prologue_ftype) (struct gdbarch *gdbarch, CORE_ADDR ip);
 extern CORE_ADDR gdbarch_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR ip);
 extern void set_gdbarch_skip_prologue (struct gdbarch *gdbarch, gdbarch_skip_prologue_ftype *skip_prologue);
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 2324138..06294ab 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -515,6 +515,13 @@ M:CORE_ADDR:integer_to_address:struct type *type, const gdb_byte *buf:type, buf
 # for instance).
 M:enum return_value_convention:return_value:struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf:function, valtype, regcache, readbuf, writebuf
 
+# Return true if the return value of function is stored in the first hidden
+# parameter.  In theory, this feature should be language-dependent, specified
+# by language and its ABI, such as C++.  Unfortunately, compiler may
+# implement it to a target-dependent feature.  So that we need such hook here
+# to be aware of this in GDB.
+m:int:return_in_first_hidden_param_p:struct type *type:type::default_return_in_first_hidden_param_p::0
+
 m:CORE_ADDR:skip_prologue:CORE_ADDR ip:ip:0:0
 M:CORE_ADDR:skip_main_prologue:CORE_ADDR ip:ip
 f:int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs:0:0
diff --git a/gdb/infcall.c b/gdb/infcall.c
index a3496d6..20a2c2b 100644
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -464,7 +464,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 {
   CORE_ADDR sp;
   struct type *values_type, *target_values_type;
-  unsigned char struct_return = 0, lang_struct_return = 0;
+  unsigned char struct_return = 0, hidden_first_param_p = 0;
   CORE_ADDR struct_addr = 0;
   struct infcall_control_state *inf_status;
   struct cleanup *inf_status_cleanup;
@@ -598,9 +598,9 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      the first argument is passed in out0 but the hidden structure
      return pointer would normally be passed in r8.  */
 
-  if (language_pass_by_reference (values_type))
+  if (gdbarch_return_in_first_hidden_param_p (gdbarch, values_type))
     {
-      lang_struct_return = 1;
+      hidden_first_param_p = 1;
 
       /* Tell the target specific argument pushing routine not to
 	 expect a value.  */
@@ -680,7 +680,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
      stack, if necessary.  Make certain that the value is correctly
      aligned.  */
 
-  if (struct_return || lang_struct_return)
+  if (struct_return || hidden_first_param_p)
     {
       int len = TYPE_LENGTH (values_type);
 
@@ -706,7 +706,7 @@ call_function_by_hand (struct value *function, int nargs, struct value **args)
 	}
     }
 
-  if (lang_struct_return)
+  if (hidden_first_param_p)
     {
       struct value **new_args;
 
@@ -1012,7 +1012,7 @@ When the function is done executing, GDB will silently stop."),
     /* Figure out the value returned by the function.  */
     retval = allocate_value (values_type);
 
-    if (lang_struct_return)
+    if (hidden_first_param_p)
       read_value_memory (retval, 0, 1, struct_addr,
 			 value_contents_raw (retval),
 			 TYPE_LENGTH (values_type));
diff --git a/gdb/m68k-tdep.c b/gdb/m68k-tdep.c
index cc1df34..b1e2cd4 100644
--- a/gdb/m68k-tdep.c
+++ b/gdb/m68k-tdep.c
@@ -1050,6 +1050,16 @@ m68k_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 }
 \f
 
+/* This is the implementation of gdbarch method
+   return_in_first_hidden_param_p.  */
+
+static int
+m68k_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
+				     struct type *type)
+{
+  return 0;
+}
+
 /* System V Release 4 (SVR4).  */
 
 void
@@ -1235,6 +1245,8 @@ m68k_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   /* Function call & return.  */
   set_gdbarch_push_dummy_call (gdbarch, m68k_push_dummy_call);
   set_gdbarch_return_value (gdbarch, m68k_return_value);
+  set_gdbarch_return_in_first_hidden_param_p (gdbarch,
+					      m68k_return_in_first_hidden_param_p);
 
 
   /* Disassembler.  */
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index c2af736..a8e31b1 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -2209,6 +2209,17 @@ sh_regset_from_core_section (struct gdbarch *gdbarch, const char *sect_name,
 
   return NULL;
 }
+
+/* This is the implementation of gdbarch method
+   return_in_first_hidden_param_p.  */
+
+static int
+sh_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
+				     struct type *type)
+{
+  return 0;
+}
+
 \f
 
 static struct gdbarch *
@@ -2260,6 +2271,8 @@ sh_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
 
   set_gdbarch_push_dummy_call (gdbarch, sh_push_dummy_call_nofpu);
+  set_gdbarch_return_in_first_hidden_param_p (gdbarch,
+					      sh_return_in_first_hidden_param_p);
 
   set_gdbarch_believe_pcc_promotion (gdbarch, 1);
 
diff --git a/gdb/tic6x-tdep.c b/gdb/tic6x-tdep.c
index 8573222..1fefcf3 100644
--- a/gdb/tic6x-tdep.c
+++ b/gdb/tic6x-tdep.c
@@ -822,6 +822,19 @@ tic6x_return_value (struct gdbarch *gdbarch, struct value *function,
 		    struct type *type, struct regcache *regcache,
 		    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
+  /* In C++, when function returns an object, even its size is small
+     enough, it stii has to be passed via reference, pointed by register
+     A3.  */
+  if (current_language->la_language == language_cplus)
+    {
+      if (type != NULL)
+	{
+	  CHECK_TYPEDEF (type);
+	  if (language_pass_by_reference (type))
+	    return RETURN_VALUE_STRUCT_CONVENTION;
+	}
+    }
+
   if (TYPE_LENGTH (type) > 8)
     return RETURN_VALUE_STRUCT_CONVENTION;
 
@@ -912,32 +925,7 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   /* The first arg passed on stack.  Mostly the first 10 args are passed by
      registers.  */
   int first_arg_on_stack = 10;
-  /* If this inf-call is a cpp method call, and return value is passed by
-     reference, this flag is set to 1, otherwise set to 0.  We need this flag
-     because computation of the return location in
-     infcall.c:call_function_by_hand is wrong for C6000 ELF ABI.  In
-     call_function_by_hand, the language is considered first, and then
-     target ABI is considered.  If language_pass_by_reference returns true,
-     the return location is passed as the first parameter to the function,
-     which is conflict with C6000 ELF ABI.  If this flag is true, we should
-     adjust args and return locations accordingly to comply with C6000 ELF
-     ABI.  */
-  int cplus_return_struct_by_reference = 0;
-
-  if (current_language->la_language == language_cplus)
-    {
-      struct type *values_type;
-
-      find_function_addr (function, &values_type);
-
-      if (values_type)
-	{
-	  CHECK_TYPEDEF (values_type);
-	  if (language_pass_by_reference (values_type))
-	    cplus_return_struct_by_reference = 1;
-	}
 
-    }
   /* Set the return address register to point to the entry point of
      the program, where a breakpoint lies in wait.  */
   regcache_cooked_write_unsigned (regcache, TIC6X_RA_REGNUM, bp_addr);
@@ -947,12 +935,6 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
      the address in A3.  */
   if (struct_return)
     regcache_cooked_write_unsigned (regcache, 3, struct_addr);
-  else if (cplus_return_struct_by_reference)
-    /* When cplus_return_struct_by_reference is 1, means local variable
-       lang_struct_return in call_function_by_hand is 1, so struct is
-       returned by reference, even STRUCT_RETURN is 0.  Note that STRUCT_ADDR
-       is still valid in this case.  */
-    regcache_cooked_write_unsigned (regcache, 3, struct_addr);
 
   /* Determine the type of this function.  */
   func_type = check_typedef (func_type);
@@ -967,10 +949,8 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   if (TYPE_VARARGS (func_type))
     first_arg_on_stack = TYPE_NFIELDS (func_type) - 1;
 
-  /* Now make space on the stack for the args.  If
-     cplus_return_struct_by_reference is 1, means GDB pass an extra parameter
-     in ARGS, which is useless here, skip it.  */
-  for (argnum = cplus_return_struct_by_reference; argnum < nargs; argnum++)
+  /* Now make space on the stack for the args.  */
+  for (argnum = 0; argnum < nargs; argnum++)
     {
       int len = align_up (TYPE_LENGTH (value_type (args[argnum])), 4);
       if (argnum >= 10 - argreg)
@@ -986,7 +966,7 @@ tic6x_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   /* Now load as many as possible of the first arguments into
      registers, and push the rest onto the stack.  Loop through args
      from first to last.  */
-  for (argnum = cplus_return_struct_by_reference; argnum < nargs; argnum++)
+  for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
       struct value *arg = args[argnum];
@@ -1207,6 +1187,16 @@ tic6x_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
   return 1;
 }
 
+/* This is the implementation of gdbarch method
+   return_in_first_hidden_param_p.  */
+
+static int
+tic6x_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
+				      struct type *type)
+{
+  return 0;
+}
+
 static struct gdbarch *
 tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 {
@@ -1365,6 +1355,9 @@ tic6x_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 
   set_gdbarch_in_function_epilogue_p (gdbarch, tic6x_in_function_epilogue_p);
 
+  set_gdbarch_return_in_first_hidden_param_p (gdbarch,
+					      tic6x_return_in_first_hidden_param_p);
+
   /* Hook in ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);
 
-- 
1.7.0.4

Re: [PATCH 1/4] New gdb arch hook: return_with_first_hidden_param_p

[Gary Benson]

Hi Mark,

Sorry for replying to this so late, I've been on leave with a new
baby!

Mark Kettenis wrote:
> > Date: Tue, 15 May 2012 14:50:58 +0800
> > From: Yao Qi <yao@codesourcery.com>
> > 
> > On 05/15/2012 01:14 AM, Joel Brobecker wrote:
> > > > > > > Can we use the fact that functions that have their first
> > > > > > > parameter be a nameless, artificial, paramter whose type
> > > > > > > is a pointer to the return type? Or would this
> > > > > > > heuristics trigger in other situations?
> > > > > 
> > > > > Yeah, I thought of that heuristics, but it still can't
> > > > > differentiate passing 3 or 4 parameters.
> > >
> > > So, you are saying that we have functions who have an extra
> > > hidden parameter used for something other than the function's
> > > return value?
> > 
> > No, the extra hidden parameter's type is the reference of the
> > function's return type.  My point is if we only examine debug
> > info, we don't know whether hidden parameter is passed, because
> > the debug info is the same regardless of hidden parameter is
> > passed or not.
> 
> I'm still very confused.  This "hidden parameter" thing sounds
> exactly like "struct_return".  So are you saying that on some
> architectures gdbarch_return_value() return
> RETUN_VALUE_REGISTER_CONVENTION when if fact for C++ (and not for
> plain C) it should return RETURN_VALUE_STRUCT_CONVENTION?

I've been investigating a bug which looks like this might be
happening:

  http://sourceware.org/bugzilla/show_bug.cgi?id=13403

I meant to email you about it before I went on leave, I was struggling
to match what it says in the comments with what I was reading in the
spec.

I don't have anything particular to say right now--I'm still trying to
figure out where I was--but I'm replying now just to let you know that
there may be an issue here.  I'll hopefully have something more useful
to say next week :)

Thanks,
Gary

-- 
http://gbenson.net/