Re: [RFA] Reference value coercion

[Daniel Jacobowitz <drow at false dot org>]

On Fri, Mar 31, 2006 at 05:28:26AM -0500, Paul Hilfinger wrote:
> at this point will eventually cause a segfault when f1 attempts to access
> a field of its argument.  This is because at the moment, since the type
> of C and the formal, P, differ, value_cast dereferences C.  This is
> a VERY bad idea generally in C++, for other reasons, but what's worse is
> that not only do we dereference C, but we then proceed to pass the 
> dereferenced value to f1 as if it were the address.  
> 
> The simple patch below to valops.c seems to solve this problem.  However,
> although I have presented this as a C++ problem, I naturally discovered it
> as an Ada problem, and it would be particularly good for a C++ person to
> consider whether other related changes to value_cast are in order.  
> 
> The patch also contains a suggested addition to the C++ testsuite.
> 
> Comments?

Hi Paul, and sorry for the delay.

The patch is not right for C++.  It works for your test, because Child
has a single base class, and therefore the values of the Child& and
Parent& are the same; but in more complicated cases, this is no longer
true.  The reference needs to be adjusted.

Also, there's some trouble with the test case: using "runto" restarts
the inferior, so runto_main followed by runto is redundant.  I think
you just want to use runto.  The step_for_stub thing is ancient, and
I don't think the testsuite works on systems that would require it any
more.

I enhanced the testcase to test that the reference was being properly
adjusted.

There's already code to do this for pointers (although it is
suspiciously simple and has a big FIXME in it...).  It's good enough
for the non-virtual case.

Does the attached work for Ada?

-- 
Daniel Jacobowitz
CodeSourcery

2006-05-05  Paul N. Hilfinger  <Hilfinger@adacore.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* infcall.c (value_arg_coerce): Use value_cast_pointers for
	references.  Avoid value_cast to a reference type.  Don't silently
	convert pointers to references.
	* valops.c (value_cast_pointers): New, based on value_cast.
	(value_cast): Use it.  Reject reference types.
	(value_ref): New.
	(typecmp): Use it.
	* value.h (value_cast_pointers, value_ref): New prototypes.

2006-05-05  Paul N. Hilfinger  <Hilfinger@adacore.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.cp/ref-params.exp: New test.
	* gdb.cp/ref-params.cc: New source file.
	* gdb.cp/Makefile.in (EXECUTABLES): Add ref-params.

Index: infcall.c
===================================================================
RCS file: /cvs/src/src/gdb/infcall.c,v
retrieving revision 1.74
diff -u -p -r1.74 infcall.c
--- infcall.c	17 Dec 2005 22:34:01 -0000	1.74
+++ infcall.c	5 May 2006 22:27:48 -0000
@@ -109,14 +109,20 @@ value_arg_coerce (struct value *arg, str
   switch (TYPE_CODE (type))
     {
     case TYPE_CODE_REF:
-      if (TYPE_CODE (arg_type) != TYPE_CODE_REF
-	  && TYPE_CODE (arg_type) != TYPE_CODE_PTR)
-	{
-	  arg = value_addr (arg);
-	  deprecated_set_value_type (arg, param_type);
-	  return arg;
-	}
-      break;
+      {
+	struct value *new_value;
+
+	if (TYPE_CODE (arg_type) == TYPE_CODE_REF)
+	  return value_cast_pointers (type, arg);
+
+	/* Cast the value to the reference's target type, and then
+	   convert it back to a reference.  This will issue an error
+	   if the value was not previously in memory - in some cases
+	   we should clearly be allowing this, but how?  */
+	new_value = value_cast (TYPE_TARGET_TYPE (type), arg);
+	new_value = value_ref (new_value);
+	return new_value;
+      }
     case TYPE_CODE_INT:
     case TYPE_CODE_CHAR:
     case TYPE_CODE_BOOL:
Index: valops.c
===================================================================
RCS file: /cvs/src/src/gdb/valops.c,v
retrieving revision 1.163
diff -u -p -r1.163 valops.c
--- valops.c	17 Dec 2005 22:34:03 -0000	1.163
+++ valops.c	5 May 2006 22:27:48 -0000
@@ -201,6 +201,70 @@ allocate_space_in_inferior (int len)
   return value_as_long (value_allocate_space_in_inferior (len));
 }
 
+/* Cast one pointer or reference type to another.  Both TYPE and
+   the type of ARG2 should be pointer types, or else both should be
+   reference types.  Returns the new pointer or reference.  */
+
+struct value *
+value_cast_pointers (struct type *type, struct value *arg2)
+{
+  struct type *type2 = check_typedef (value_type (arg2));
+  struct type *t1 = check_typedef (TYPE_TARGET_TYPE (type));
+  struct type *t2 = check_typedef (TYPE_TARGET_TYPE (type2));
+
+  if (TYPE_CODE (t1) == TYPE_CODE_STRUCT
+      && TYPE_CODE (t2) == TYPE_CODE_STRUCT
+      && !value_logical_not (arg2))
+    {
+      struct value *v;
+
+      /* Look in the type of the source to see if it contains the
+	 type of the target as a superclass.  If so, we'll need to
+	 offset the pointer rather than just change its type.  */
+      if (TYPE_NAME (t1) != NULL)
+	{
+	  struct value *v2;
+
+	  if (TYPE_CODE (type2) == TYPE_CODE_REF)
+	    v2 = coerce_ref (arg2);
+	  else
+	    v2 = value_ind (arg2);
+	  v = search_struct_field (type_name_no_tag (t1),
+				   v2, 0, t2, 1);
+	  if (v)
+	    {
+	      v = value_addr (v);
+	      deprecated_set_value_type (v, type);
+	      return v;
+	    }
+	}
+
+      /* Look in the type of the target to see if it contains the
+	 type of the source as a superclass.  If so, we'll need to
+	 offset the pointer rather than just change its type.
+	 FIXME: This fails silently with virtual inheritance.  */
+      if (TYPE_NAME (t2) != NULL)
+	{
+	  v = search_struct_field (type_name_no_tag (t2),
+				   value_zero (t1, not_lval), 0, t1, 1);
+	  if (v)
+	    {
+	      CORE_ADDR addr2 = value_as_address (arg2);
+	      addr2 -= (VALUE_ADDRESS (v)
+			+ value_offset (v)
+			+ value_embedded_offset (v));
+	      return value_from_pointer (type, addr2);
+	    }
+	}
+    }
+
+  /* No superclass found, just change the pointer type.  */
+  deprecated_set_value_type (arg2, type);
+  arg2 = value_change_enclosing_type (arg2, type);
+  set_value_pointed_to_offset (arg2, 0);	/* pai: chk_val */
+  return arg2;
+}
+
 /* Cast value ARG2 to type TYPE and return as a value.
    More general than a C cast: accepts any two types of the same length,
    and if ARG2 is an lvalue it can be cast into anything at all.  */
@@ -224,6 +288,10 @@ value_cast (struct type *type, struct va
   arg2 = coerce_ref (arg2);
   type2 = check_typedef (value_type (arg2));
 
+  /* You can't cast to a reference type.  See value_cast_pointers
+     instead.  */
+  gdb_assert (code1 != TYPE_CODE_REF);
+
   /* A cast to an undetermined-length array_type, such as (TYPE [])OBJECT,
      is treated like a cast to (TYPE [N])OBJECT,
      where N is sizeof(OBJECT)/sizeof(TYPE). */
@@ -369,50 +437,8 @@ value_cast (struct type *type, struct va
   else if (TYPE_LENGTH (type) == TYPE_LENGTH (type2))
     {
       if (code1 == TYPE_CODE_PTR && code2 == TYPE_CODE_PTR)
-	{
-	  struct type *t1 = check_typedef (TYPE_TARGET_TYPE (type));
-	  struct type *t2 = check_typedef (TYPE_TARGET_TYPE (type2));
-	  if (TYPE_CODE (t1) == TYPE_CODE_STRUCT
-	      && TYPE_CODE (t2) == TYPE_CODE_STRUCT
-	      && !value_logical_not (arg2))
-	    {
-	      struct value *v;
+	return value_cast_pointers (type, arg2);
 
-	      /* Look in the type of the source to see if it contains the
-	         type of the target as a superclass.  If so, we'll need to
-	         offset the pointer rather than just change its type.  */
-	      if (TYPE_NAME (t1) != NULL)
-		{
-		  v = search_struct_field (type_name_no_tag (t1),
-					   value_ind (arg2), 0, t2, 1);
-		  if (v)
-		    {
-		      v = value_addr (v);
-		      deprecated_set_value_type (v, type);
-		      return v;
-		    }
-		}
-
-	      /* Look in the type of the target to see if it contains the
-	         type of the source as a superclass.  If so, we'll need to
-	         offset the pointer rather than just change its type.
-	         FIXME: This fails silently with virtual inheritance.  */
-	      if (TYPE_NAME (t2) != NULL)
-		{
-		  v = search_struct_field (type_name_no_tag (t2),
-				       value_zero (t1, not_lval), 0, t1, 1);
-		  if (v)
-		    {
-                      CORE_ADDR addr2 = value_as_address (arg2);
-                      addr2 -= (VALUE_ADDRESS (v)
-                                + value_offset (v)
-                                + value_embedded_offset (v));
-                      return value_from_pointer (type, addr2);
-		    }
-		}
-	    }
-	  /* No superclass found, just fall through to change ptr type.  */
-	}
       deprecated_set_value_type (arg2, type);
       arg2 = value_change_enclosing_type (arg2, type);
       set_value_pointed_to_offset (arg2, 0);	/* pai: chk_val */
@@ -886,6 +912,22 @@ value_addr (struct value *arg1)
   return arg2;
 }
 
+/* Return a reference value for the object for which ARG1 is the contents.  */
+
+struct value *
+value_ref (struct value *arg1)
+{
+  struct value *arg2;
+
+  struct type *type = check_typedef (value_type (arg1));
+  if (TYPE_CODE (type) == TYPE_CODE_REF)
+    return arg1;
+
+  arg2 = value_addr (arg1);
+  deprecated_set_value_type (arg2, lookup_reference_type (type));
+  return arg2;
+}
+
 /* Given a value of a pointer type, apply the C unary * operator to it.  */
 
 struct value *
@@ -1106,7 +1148,7 @@ typecmp (int staticp, int varargs, int n
 	  if (TYPE_CODE (tt2) == TYPE_CODE_ARRAY)
 	    t2[i] = value_coerce_array (t2[i]);
 	  else
-	    t2[i] = value_addr (t2[i]);
+	    t2[i] = value_ref (t2[i]);
 	  continue;
 	}
 
Index: value.h
===================================================================
RCS file: /cvs/src/src/gdb/value.h,v
retrieving revision 1.91
diff -u -p -r1.91 value.h
--- value.h	31 Mar 2006 10:36:18 -0000	1.91
+++ value.h	5 May 2006 22:27:48 -0000
@@ -325,6 +325,8 @@ extern struct value *value_ind (struct v
 
 extern struct value *value_addr (struct value *arg1);
 
+extern struct value *value_ref (struct value *arg1);
+
 extern struct value *value_assign (struct value *toval,
 				   struct value *fromval);
 
@@ -367,6 +369,8 @@ extern struct type *value_rtti_target_ty
 extern struct value *value_full_object (struct value *, struct type *, int,
 					int, int);
 
+extern struct value *value_cast_pointers (struct type *, struct value *);
+
 extern struct value *value_cast (struct type *type, struct value *arg2);
 
 extern struct value *value_zero (struct type *type, enum lval_type lv);
Index: testsuite/gdb.cp/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/Makefile.in,v
retrieving revision 1.1
diff -u -p -r1.1 Makefile.in
--- testsuite/gdb.cp/Makefile.in	23 Aug 2003 03:55:59 -0000	1.1
+++ testsuite/gdb.cp/Makefile.in	5 May 2006 22:27:49 -0000
@@ -3,7 +3,8 @@ srcdir = @srcdir@
 
 EXECUTABLES = ambiguous annota2 anon-union cplusfuncs cttiadd \
 	derivation inherit local member-ptr method misc \
-        overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace ref-types
+        overload ovldbreak ref-typ ref-typ2 templates userdef virtfunc namespace \
+	ref-types ref-params
 
 all info install-info dvi install uninstall installcheck check:
 	@echo "Nothing to be done for $@..."
Index: testsuite/gdb.cp/ref-params.cc
===================================================================
RCS file: testsuite/gdb.cp/ref-params.cc
diff -N testsuite/gdb.cp/ref-params.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/ref-params.cc	5 May 2006 22:27:49 -0000
@@ -0,0 +1,80 @@
+/* This test script is part of GDB, the GNU debugger.
+
+   Copyright 2006
+   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 2 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, write to the Free Software
+   Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+   */
+
+/* Author: Paul N. Hilfinger, AdaCore Inc. */
+
+struct Parent {
+  Parent (int id0) : id(id0) { }
+  int id;
+};
+
+struct Child : public Parent {
+  Child (int id0) : Parent(id0) { }
+};
+
+int f1(Parent& R)
+{
+  return R.id;			/* Set breakpoint marker3 here.  */
+}
+
+int f2(Child& C)
+{
+  return f1(C);			/* Set breakpoint marker2 here.  */
+}
+
+struct OtherParent {
+  OtherParent (int other_id0) : other_id(other_id0) { }
+  int other_id;
+};
+
+struct MultiChild : public Parent, OtherParent {
+  MultiChild (int id0) : Parent(id0), OtherParent(id0 * 2) { }
+};
+
+int mf1(OtherParent& R)
+{
+  return R.other_id;
+}
+
+int mf2(MultiChild& C)
+{
+  return mf1(C);
+}
+
+int main(void) 
+{
+  Child Q(42);
+  Child& QR = Q;
+
+  #ifdef usestubs
+     set_debug_traps();
+     breakpoint();
+  #endif
+
+  /* Set breakpoint marker1 here.  */
+
+  f2(Q);
+  f2(QR);
+
+  MultiChild MQ(53);
+  MultiChild& MQR = MQ;
+
+  mf2(MQ);			/* Set breakpoint MQ here.  */
+}
Index: testsuite/gdb.cp/ref-params.exp
===================================================================
RCS file: testsuite/gdb.cp/ref-params.exp
diff -N testsuite/gdb.cp/ref-params.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/ref-params.exp	5 May 2006 22:27:49 -0000
@@ -0,0 +1,79 @@
+# Tests for reference parameters of types and their subtypes in GDB.
+# Copyright 2006 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 2 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, write to the Free Software
+# Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.  
+
+# written by Paul N. Hilfinger (Hilfinger@adacore.com)
+
+if $tracelevel then {
+        strace $tracelevel
+        }
+
+#
+# test running programs
+#
+set prms_id 0
+set bug_id 0
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile "ref-params"
+set srcfile ${testfile}.cc
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
+     gdb_suppress_entire_file "Testcase compile failed, so all tests in this file will automatically fail."
+}
+
+gdb_exit
+
+proc gdb_start_again { text } {
+    global srcdir
+    global subdir
+    global binfile
+    global srcfile
+
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load ${binfile}
+
+    runto ${srcfile}:[gdb_get_line_number $text]
+}
+
+gdb_start_again "marker1 here"
+gdb_test "print Q" ".*id = 42.*" "print value of a Child in main"
+gdb_test "print f1(Q)" ".* = 42.*" "print value of f1 on Child in main"
+gdb_test "print f2(Q)" ".* = 42.*" "print value of f2 on Child in main"
+
+gdb_start_again "marker1 here"
+gdb_test "print f1(QR)" ".* = 42.*" "print value of f1 on (Child&) in main"
+
+gdb_start_again "marker1 here"
+gdb_test "print f2(QR)" ".* = 42.*" "print value of f2 on (Child&) in main"
+
+gdb_start_again "marker2 here"
+gdb_test "print C" ".*id = 42.*" "print value of Child& in f2"
+gdb_test "print f1(C)" ".* = 42.*" "print value of f1 on Child& in f2"
+
+gdb_start_again "marker3 here"
+gdb_test "print R" ".*id = 42.*" "print value of Parent& in f1"
+
+gdb_start_again "breakpoint MQ here"
+gdb_test "print f1(MQ)" ".* = 53"
+gdb_test "print mf1(MQ)" ".* = 106"
+gdb_test "print mf2(MQ)" ".* = 106"
+gdb_test "print f1(MQR)" ".* = 53"
+gdb_test "print mf1(MQR)" ".* = 106"
+gdb_test "print mf2(MQR)" ".* = 106"