From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26202 invoked by alias); 5 May 2006 23:19:06 -0000 Received: (qmail 26193 invoked by uid 22791); 5 May 2006 23:19:04 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Fri, 05 May 2006 23:19:00 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1Fc9ZY-0002cW-2W; Fri, 05 May 2006 19:18:56 -0400 Date: Fri, 05 May 2006 23:19:00 -0000 From: Daniel Jacobowitz To: Paul Hilfinger Cc: gdb-patches@sourceware.org Subject: Re: [RFA] Reference value coercion Message-ID: <20060505231856.GB31029@nevyn.them.org> Mail-Followup-To: Paul Hilfinger , gdb-patches@sourceware.org References: <20060331102826.2795648CF65@nile.gnat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20060331102826.2795648CF65@nile.gnat.com> User-Agent: Mutt/1.5.8i X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-05/txt/msg00124.txt.bz2 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 Daniel Jacobowitz * 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 Daniel Jacobowitz * 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"