MI/C++/references fixup

MI/C++/references fixup

[Vladimir Prus]

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


As noted by Nick, my varobj laziness patch has broken C++ references 
updates -- now -var-update does not detect the situation when a reference 
variable just got initialized, for example:

	int x = 4;
	int &rx = x;

after executing the second line, -var-update does not say that varobj 
corresponding to rx has change.

References were already broken, in a different way, before my 
change -- -var-update would always list reference variable as changed. That 
was because gdb was comparing undereferenced old value with dereferenced new 
value -- that obviously never compare equal.

Also, display of references includes the address, like this:

	-var-evaluate-expression var1
	^done,value="@0xbff72ebc: 4"

We discussed this at length some time ago:

	http://thread.gmane.org/gmane.comp.gdb.patches/28414/

The conclusion was that this "address prefix" is not necessary. In fact, both 
KDevelop and Eclipse explicitly remove it. Back then, it was decided that 
change would be hard. But recent varobj refactorings make this change 
straight-forward.

Attached is the patch that:

	- Makes -var-update property report reference variables --
	only when the referred-to value actually changed.

	- Removes the "@ADDRESS" output for reference variables.

	- Adds tests for references. 

The patch and the new files are attached.

- Volodya

	* varobj.c (varobj_create): Don't call release_value.
	(varobj_set_value): Likewise.
	(install_new_value): Call coerce_ref and release_value
	on the value. Add asserts.

	testsuite/
	* lib/mi-support.exp
	(mi_runto): Accept "()" after function name.
	(mi_create_varobj): New function.
	(mi_varobj_update): New function.
	(mi_Check_varobj_value): New function.
	* gdb.mi/mi-cpp.exp: New file.
	* gdb.mi/mi-cpp.cpp: New file.






[-- Attachment #2: 00REFERENCES.diff --]
[-- Type: text/x-diff, Size: 4582 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.62
diff -u -p -r1.62 varobj.c
--- varobj.c	29 Nov 2006 06:41:13 -0000	1.62
+++ varobj.c	29 Nov 2006 08:52:22 -0000
@@ -512,10 +512,7 @@ varobj_create (char *objname,
 	   right type.  */
 	value = evaluate_type (var->root->exp);
 
-      release_value (value);
-
       var->type = value_type (value);
-
       install_new_value (var, value, 1 /* Initial assignment */);
 
       /* Set language info */
@@ -847,9 +844,7 @@ varobj_set_value (struct varobj *var, ch
 	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
-
-      release_value (val);
-      
+     
       /* If the value has changed, record it, so that next -var-update can
 	 report this change.  If a variable had a value of '1', we've set it
 	 to '333' and then set again to '1', when -var-update will report this
@@ -901,7 +896,10 @@ varobj_list (struct varobj ***varlist)
    and return 0.
    Otherwise, assign the value and if type_changeable returns non-zero,
    find if the new value is different from the current value.
-   Return 1 if so, and 0 if the values are equal.  */
+   Return 1 if so, and 0 if the values are equal.  
+
+   The VALUE parameter should not be released -- the function will
+   take care of releasing it when needed.  */
 static int
 install_new_value (struct varobj *var, struct value *value, int initial)
 { 
@@ -917,6 +915,15 @@ install_new_value (struct varobj *var, s
   changeable = type_changeable (var);
   need_to_fetch = changeable;
 
+  /* We are not interested in address of references, and given
+     that in C++ reference is not rebindable, it cannot
+     meaningfully change.  So, get hold of the real value.  */
+  if (value)
+    {
+      value = coerce_ref (value);
+      release_value (value);
+    }
+
   if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
     /* For unions, we need to fetch the value implicitly because
        of implementation of union member fetch.  When gdb
@@ -985,6 +992,8 @@ install_new_value (struct varobj *var, s
     value_free (var->value);
   var->value = value;
   var->updated = 0;
+  
+  gdb_assert (!var->value || value_type (var->value));
 
   return changed;
 }
Index: testsuite/lib/mi-support.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/mi-support.exp,v
retrieving revision 1.33
diff -u -p -r1.33 mi-support.exp
--- testsuite/lib/mi-support.exp	24 Jul 2006 20:38:08 -0000	1.33
+++ testsuite/lib/mi-support.exp	29 Nov 2006 08:52:23 -0000
@@ -869,7 +869,7 @@ proc mi_runto {func} {
 
   set test "mi runto $func"
   mi_gdb_test "200-break-insert $func" \
-    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
+    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\(\\\(\\\)\)?\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
     "breakpoint at $func"
 
   if {![regexp {number="[0-9]+"} $expect_out(buffer) str]
@@ -998,3 +998,40 @@ proc mi0_continue_to { bkptno func args 
     mi0_execute_to "exec-continue" "breakpoint-hit\",bkptno=\"$bkptno" \
 	"$func" "$args" "$file" "$line" "" "$test"
 }
+
+# Creates varobj named NAME for EXPRESSION.
+# Name cannot be "-".
+proc mi_create_varobj { name expression } {
+    mi_gdb_test "-var-create $name * $expression" \
+        "\\^done,name=\"$name\",numchild=\"\[0-9\]+\",type=.*" \
+        "-var-create $name * $expression"
+}
+
+# Updates varobj named NAME and checks that all varobjs in EXPECTED
+# are reported as updated, and no other varobj is updated.
+# Assumes that no varobj is out of scope and that no varobj changes
+# types.
+proc mi_varobj_update { name expected } {
+    set er "\\^done,changelist=\\\["
+    set first 1
+    foreach item $expected {
+        set v "{name=\"$item\",in_scope=\"true\",type_changed=\"false\"}"
+        if {$first} {
+            set er "$er$v"
+        } else {
+            set er "$er,$v"
+        }
+    }
+    set er "$er\\\]"
+
+    verbose -log "Expecting: $er" 2
+    mi_gdb_test "-var-update $name" $er \
+        "-var-update $name: expect $expected"
+}
+
+proc mi_check_varobj_value { name value } {
+
+    mi_gdb_test "-var-evaluate-expression $name" \
+	"\\^done,value=\"$value\"" \
+	"-var-evaluate-expression $name: expect $value"
+}
\ No newline at end of file

[-- Attachment #3: mi-cpp.exp --]
[-- Type: text/plain, Size: 2116 bytes --]

# Copyright 2002, 2003 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.  

# Please email any bugs, comments, and/or additions to this file to:
# bug-gdb@prep.ai.mit.edu

if { [skip_cplus_tests] } { continue }

load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile mi-cpp
set srcfile "$testfile.cpp"
set binfile $objdir/$subdir/$testfile

if [get_compiler_info ${binfile} "c++"] {
    return -1;
}

if {[gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug c++}] != ""} {
  untested $testfile.exp
  return -1
}

mi_gdb_load ${binfile}

# Test that children of classes are properly reported

# Run to main
mi_runto reference_update_tests

mi_create_varobj "RX" "rx"

set x_assignment [gdb_get_line_number "x = 567;"]
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment-1] \
    "step to x assignment"
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment] \
    "step to x assignment"

mi_varobj_update RX {RX}

mi_check_varobj_value RX 167

# Execute the first 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+1] \
    "step to x assignment"

mi_varobj_update RX {RX}
mi_check_varobj_value RX 567

# Execute the second 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+2] \
    "step to x assignment"

mi_varobj_update RX {}

mi_gdb_exit
return 0

[-- Attachment #4: mi-cpp.cpp --]
[-- Type: text/x-c++src, Size: 146 bytes --]


void reference_update_tests ()
{
  int x = 167;
  int& rx = x;
  x = 567;
  x = 567;
}

int main ()
{
  reference_update_tests ();
  return 0;
}

Re: MI/C++/references fixup

[Vladimir Prus]

Vladimir Prus wrote:

> 
> As noted by Nick, my varobj laziness patch has broken C++ references
> updates -- now -var-update does not detect the situation when a reference
> variable just got initialized, for example:
> 
> int x = 4;
> int &rx = x;

I've got that wrong -- the initialization of reference is detected, but the
change of the value itself is not.

- Volodya

Re: MI/C++/references fixup

[Daniel Jacobowitz]

On Wed, Nov 29, 2006 at 12:15:21PM +0300, Vladimir Prus wrote:
> We discussed this at length some time ago:
> 
> 	http://thread.gmane.org/gmane.comp.gdb.patches/28414/
> 
> The conclusion was that this "address prefix" is not necessary. In fact, both 
> KDevelop and Eclipse explicitly remove it. Back then, it was decided that 
> change would be hard. But recent varobj refactorings make this change 
> straight-forward.

Did we?  I believe you, but I can't find it in the thread, and I don't
remember.

> +  /* We are not interested in address of references, and given
> +     that in C++ reference is not rebindable, it cannot
> +     meaningfully change.  So, get hold of the real value.  */

in the address of a reference, in C++ a reference.

In C++ it can't meaningfully change.  In a program, though, it can;
once when it's initialized, and again if something scribbles on the
stack.  And that might be what you're trying to debug.  So, I'm
a little wary of this; it seems to me that we ought to check for both
changes in the address and value (sort of like we do for watchpoints).

Per your other message, I'd rather not check in the patch without this;
want me to try it?

>    mi_gdb_test "200-break-insert $func" \
> -    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
> +    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\(\\\(\\\)\)?\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
>      "breakpoint at $func"

Probably ought to allow (.*) for the function in case it has arguments
(or the C++ demangler gives us back a (void)).

> +proc mi_check_varobj_value { name value } {
> +
> +    mi_gdb_test "-var-evaluate-expression $name" \
> +	"\\^done,value=\"$value\"" \
> +	"-var-evaluate-expression $name: expect $value"
> +}
> \ No newline at end of file

Please add newline :-)

> # Copyright 2002, 2003 Free Software Foundation, Inc.

And update copyright years.

> # Please email any bugs, comments, and/or additions to this file to:
> # bug-gdb@prep.ai.mit.edu

And skip that.  We really should remove it from every file.

And copyright message on new tests please.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI/C++/references fixup

[Vladimir Prus]

On Wednesday 29 November 2006 17:01, Daniel Jacobowitz wrote:
> On Wed, Nov 29, 2006 at 12:15:21PM +0300, Vladimir Prus wrote:
> > We discussed this at length some time ago:
> >
> > 	http://thread.gmane.org/gmane.comp.gdb.patches/28414/
> >
> > The conclusion was that this "address prefix" is not necessary. In fact,
> > both KDevelop and Eclipse explicitly remove it. Back then, it was decided
> > that change would be hard. But recent varobj refactorings make this
> > change straight-forward.
>
> Did we?  I believe you, but I can't find it in the thread, and I don't
> remember.

The last thing I remember from this thread is that we tried, but failed.

> > +  /* We are not interested in address of references, and given
> > +     that in C++ reference is not rebindable, it cannot
> > +     meaningfully change.  So, get hold of the real value.  */
>
> in the address of a reference, in C++ a reference.
>
> In C++ it can't meaningfully change.  In a program, though, it can;
> once when it's initialized, and again if something scribbles on the
> stack.  And that might be what you're trying to debug.  So, I'm
> a little wary of this; it seems to me that we ought to check for both
> changes in the address and value (sort of like we do for watchpoints).

In practice, if the address changes, the value also changes, so the user can 
notice. Second, if user really wants to get the address, he can do that 
with "&whatever".

Finally, what's the point of displaying address, if Eclipse and KDevelop strip 
it, and I think Nick wants it to be gone too?

>
> Per your other message, I'd rather not check in the patch without this;
> want me to try it?
>
> >    mi_gdb_test "200-break-insert $func" \
> > -   
> > "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",
> >enabled=\"y\",addr=\"$hex\",func=\"$func\",file=\".*\",line=\"\[0-9\]*\",t
> >imes=\"0\"\}" \ +   
> > "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",
> >enabled=\"y\",addr=\"$hex\",func=\"$func\(\\\(\\\)\)?\",file=\".*\",line=\
> >"\[0-9\]*\",times=\"0\"\}" \ "breakpoint at $func"
>
> Probably ought to allow (.*) for the function in case it has arguments
> (or the C++ demangler gives us back a (void)).

Okay.

> > +proc mi_check_varobj_value { name value } {
> > +
> > +    mi_gdb_test "-var-evaluate-expression $name" \
> > +	"\\^done,value=\"$value\"" \
> > +	"-var-evaluate-expression $name: expect $value"
> > +}
> > \ No newline at end of file
>
> Please add newline :-)
>
> > # Copyright 2002, 2003 Free Software Foundation, Inc.
>
> And update copyright years.
>
> > # Please email any bugs, comments, and/or additions to this file to:
> > # bug-gdb@prep.ai.mit.edu
>
> And skip that.  We really should remove it from every file.
>
> And copyright message on new tests please.

Okay.

- Volodya

Re: MI/C++/references fixup

[Daniel Jacobowitz]

On Wed, Nov 29, 2006 at 05:15:04PM +0300, Vladimir Prus wrote:
> > > +  /* We are not interested in address of references, and given
> > > +     that in C++ reference is not rebindable, it cannot
> > > +     meaningfully change.  So, get hold of the real value.  */
> >
> > in the address of a reference, in C++ a reference.
> >
> > In C++ it can't meaningfully change.  In a program, though, it can;
> > once when it's initialized, and again if something scribbles on the
> > stack.  And that might be what you're trying to debug.  So, I'm
> > a little wary of this; it seems to me that we ought to check for both
> > changes in the address and value (sort of like we do for watchpoints).
> 
> In practice, if the address changes, the value also changes, so the user can 
> notice. Second, if user really wants to get the address, he can do that 
> with "&whatever".

I suppose that's true.  Want to post an updated patch, and we'll see if
anyone has a reason to keep it?  We're leaving the CLI as it was, this
time.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI/C++/references fixup

[Vladimir Prus]

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

On Wednesday 29 November 2006 17:24, Daniel Jacobowitz wrote:
> On Wed, Nov 29, 2006 at 05:15:04PM +0300, Vladimir Prus wrote:
> > > > +  /* We are not interested in address of references, and given
> > > > +     that in C++ reference is not rebindable, it cannot
> > > > +     meaningfully change.  So, get hold of the real value.  */
> > >
> > > in the address of a reference, in C++ a reference.
> > >
> > > In C++ it can't meaningfully change.  In a program, though, it can;
> > > once when it's initialized, and again if something scribbles on the
> > > stack.  And that might be what you're trying to debug.  So, I'm
> > > a little wary of this; it seems to me that we ought to check for both
> > > changes in the address and value (sort of like we do for watchpoints).
> >
> > In practice, if the address changes, the value also changes, so the user
> > can notice. Second, if user really wants to get the address, he can do
> > that with "&whatever".
>
> I suppose that's true.  Want to post an updated patch, and we'll see if
> anyone has a reason to keep it?  We're leaving the CLI as it was, this
> time.

Here's it.

- Volodya

        * varobj.c (varobj_create): Don't call release_value.
        (varobj_set_value): Likewise.
        (install_new_value): Call coerce_ref and release_value
        on the value. Add asserts.

        testsuite/
        * lib/mi-support.exp
        (mi_runto): Accept "()" after function name.
        (mi_create_varobj): New function.
        (mi_varobj_update): New function.
        (mi_Check_varobj_value): New function.
        * gdb.mi/mi-cpp.exp: New file.
        * gdb.mi/mi-cpp.cpp: New file.

[-- Attachment #2: 00REFERENCES.diff --]
[-- Type: text/x-diff, Size: 4562 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.62
diff -u -p -r1.62 varobj.c
--- varobj.c	29 Nov 2006 06:41:13 -0000	1.62
+++ varobj.c	29 Nov 2006 14:37:11 -0000
@@ -512,10 +512,7 @@ varobj_create (char *objname,
 	   right type.  */
 	value = evaluate_type (var->root->exp);
 
-      release_value (value);
-
       var->type = value_type (value);
-
       install_new_value (var, value, 1 /* Initial assignment */);
 
       /* Set language info */
@@ -847,9 +844,7 @@ varobj_set_value (struct varobj *var, ch
 	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
-
-      release_value (val);
-      
+     
       /* If the value has changed, record it, so that next -var-update can
 	 report this change.  If a variable had a value of '1', we've set it
 	 to '333' and then set again to '1', when -var-update will report this
@@ -901,7 +896,10 @@ varobj_list (struct varobj ***varlist)
    and return 0.
    Otherwise, assign the value and if type_changeable returns non-zero,
    find if the new value is different from the current value.
-   Return 1 if so, and 0 if the values are equal.  */
+   Return 1 if so, and 0 if the values are equal.  
+
+   The VALUE parameter should not be released -- the function will
+   take care of releasing it when needed.  */
 static int
 install_new_value (struct varobj *var, struct value *value, int initial)
 { 
@@ -917,6 +915,15 @@ install_new_value (struct varobj *var, s
   changeable = type_changeable (var);
   need_to_fetch = changeable;
 
+  /* We are not interested in the address of references, and given
+     that in C++ a reference is not rebindable, it cannot
+     meaningfully change.  So, get hold of the real value.  */
+  if (value)
+    {
+      value = coerce_ref (value);
+      release_value (value);
+    }
+
   if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
     /* For unions, we need to fetch the value implicitly because
        of implementation of union member fetch.  When gdb
@@ -985,6 +992,8 @@ install_new_value (struct varobj *var, s
     value_free (var->value);
   var->value = value;
   var->updated = 0;
+  
+  gdb_assert (!var->value || value_type (var->value));
 
   return changed;
 }
Index: testsuite/lib/mi-support.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/mi-support.exp,v
retrieving revision 1.33
diff -u -p -r1.33 mi-support.exp
--- testsuite/lib/mi-support.exp	24 Jul 2006 20:38:08 -0000	1.33
+++ testsuite/lib/mi-support.exp	29 Nov 2006 14:37:12 -0000
@@ -869,7 +869,7 @@ proc mi_runto {func} {
 
   set test "mi runto $func"
   mi_gdb_test "200-break-insert $func" \
-    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
+    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\(\\\(.*\\\)\)?\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
     "breakpoint at $func"
 
   if {![regexp {number="[0-9]+"} $expect_out(buffer) str]
@@ -998,3 +998,40 @@ proc mi0_continue_to { bkptno func args 
     mi0_execute_to "exec-continue" "breakpoint-hit\",bkptno=\"$bkptno" \
 	"$func" "$args" "$file" "$line" "" "$test"
 }
+
+# Creates varobj named NAME for EXPRESSION.
+# Name cannot be "-".
+proc mi_create_varobj { name expression } {
+    mi_gdb_test "-var-create $name * $expression" \
+        "\\^done,name=\"$name\",numchild=\"\[0-9\]+\",type=.*" \
+        "-var-create $name * $expression"
+}
+
+# Updates varobj named NAME and checks that all varobjs in EXPECTED
+# are reported as updated, and no other varobj is updated.
+# Assumes that no varobj is out of scope and that no varobj changes
+# types.
+proc mi_varobj_update { name expected } {
+    set er "\\^done,changelist=\\\["
+    set first 1
+    foreach item $expected {
+        set v "{name=\"$item\",in_scope=\"true\",type_changed=\"false\"}"
+        if {$first} {
+            set er "$er$v"
+        } else {
+            set er "$er,$v"
+        }
+    }
+    set er "$er\\\]"
+
+    verbose -log "Expecting: $er" 2
+    mi_gdb_test "-var-update $name" $er \
+        "-var-update $name: expect $expected"
+}
+
+proc mi_check_varobj_value { name value } {
+
+    mi_gdb_test "-var-evaluate-expression $name" \
+	"\\^done,value=\"$value\"" \
+	"-var-evaluate-expression $name: expect $value"
+}

[-- Attachment #3: mi-cpp.cpp --]
[-- Type: text/x-c++src, Size: 915 bytes --]

/* 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.  
*/

void reference_update_tests ()
{
  int x = 167;
  int& rx = x;
  x = 567;
  x = 567;
}

int main ()
{
  reference_update_tests ();
  return 0;
}

[-- Attachment #4: mi-cpp.exp --]
[-- Type: text/plain, Size: 2014 bytes --]

# 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.  

if { [skip_cplus_tests] } { continue }

load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile mi-cpp
set srcfile "$testfile.cpp"
set binfile $objdir/$subdir/$testfile

if [get_compiler_info ${binfile} "c++"] {
    return -1;
}

if {[gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug c++}] != ""} {
  untested $testfile.exp
  return -1
}

mi_gdb_load ${binfile}

# Test that children of classes are properly reported

# Run to main
mi_runto reference_update_tests

mi_create_varobj "RX" "rx"

set x_assignment [gdb_get_line_number "x = 567;"]
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment-1] \
    "step to x assignment"
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment] \
    "step to x assignment"

mi_varobj_update RX {RX}

mi_check_varobj_value RX 167

# Execute the first 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+1] \
    "step to x assignment"

mi_varobj_update RX {RX}
mi_check_varobj_value RX 567

# Execute the second 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+2] \
    "step to x assignment"

mi_varobj_update RX {}

mi_gdb_exit
return 0

Re: MI/C++/references fixup

[Nick Roberts]

 > > > > In C++ it can't meaningfully change.  In a program, though, it can;
 > > > > once when it's initialized, and again if something scribbles on the
 > > > > stack.  And that might be what you're trying to debug.  So, I'm
 > > > > a little wary of this; it seems to me that we ought to check for both
 > > > > changes in the address and value (sort of like we do for watchpoints).

I'm not that familiar with references but presumably the idea is not to have
to think about the address otherwise you might as well use a pointer.

 > > > In practice, if the address changes, the value also changes, so the user
 > > > can notice. Second, if user really wants to get the address, he can do
 > > > that with "&whatever".
 > >
 > > I suppose that's true.  Want to post an updated patch, and we'll see if
 > > anyone has a reason to keep it?  We're leaving the CLI as it was, this
 > > time.
 > 
 > Here's it.

 > - Volodya

 >         * varobj.c (varobj_create): Don't call release_value.
 >         (varobj_set_value): Likewise.
 >         (install_new_value): Call coerce_ref and release_value
 >         on the value. Add asserts.

This changes to varobj.c look good to me.

        * gdb.mi/mi-cpp.exp: New file.
        * gdb.mi/mi-cpp.cpp: New file.

gdb.mi/mi-var-cp.exp?
gdb.mi/mi-var-cp.cc?

Its for variable objects, and for consistency.  The testsuite has the directory
gdb.cp and it's populated with *.cc files


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: MI/C++/references fixup

[Vladimir Prus]

On Thursday 30 November 2006 02:54, you wrote:
>  > > > > In C++ it can't meaningfully change.  In a program, though, it
>  > > > > can; once when it's initialized, and again if something scribbles
>  > > > > on the stack.  And that might be what you're trying to debug.  So,
>  > > > > I'm a little wary of this; it seems to me that we ought to check
>  > > > > for both changes in the address and value (sort of like we do for
>  > > > > watchpoints).
>
> I'm not that familiar with references but presumably the idea is not to
> have to think about the address otherwise you might as well use a pointer.
>
>  > > > In practice, if the address changes, the value also changes, so the
>  > > > user can notice. Second, if user really wants to get the address, he
>  > > > can do that with "&whatever".
>  > >
>  > > I suppose that's true.  Want to post an updated patch, and we'll see
>  > > if anyone has a reason to keep it?  We're leaving the CLI as it was,
>  > > this time.
>  >
>  > Here's it.
>  >
>  > - Volodya
>  >
>  >         * varobj.c (varobj_create): Don't call release_value.
>  >         (varobj_set_value): Likewise.
>  >         (install_new_value): Call coerce_ref and release_value
>  >         on the value. Add asserts.
>
> This changes to varobj.c look good to me.
>
>         * gdb.mi/mi-cpp.exp: New file.
>         * gdb.mi/mi-cpp.cpp: New file.
>
> gdb.mi/mi-var-cp.exp?
> gdb.mi/mi-var-cp.cc?
>
> Its for variable objects, and for consistency.  The testsuite has the
> directory gdb.cp and it's populated with *.cc files

I've no problems with 'cp' and '.cc'. I don't think that '-var-' is good -- 
now, there is just single MI test dealing with C++, so I want a testcase that 
will accumulate all C++ specific things, not necessary related to variable 
objects. But I don't care much.

- Volodya

Re: MI/C++/references fixup

[Daniel Jacobowitz]

On Wed, Nov 29, 2006 at 05:41:13PM +0300, Vladimir Prus wrote:
> +# Creates varobj named NAME for EXPRESSION.
> +# Name cannot be "-".
> +proc mi_create_varobj { name expression } {

> +# Updates varobj named NAME and checks that all varobjs in EXPECTED
> +# are reported as updated, and no other varobj is updated.
> +# Assumes that no varobj is out of scope and that no varobj changes
> +# types.
> +proc mi_varobj_update { name expected } {

> +proc mi_check_varobj_value { name value } {

I would recommend giving these an argument for the test name, as some
of the other helper functions do.  Otherwise, if you call them twice in
a row with the same arguments (which your test does) then you've got
two tests with the same name.

On Thu, Nov 30, 2006 at 10:31:07AM +0300, Vladimir Prus wrote:
> >         * gdb.mi/mi-cpp.cpp: New file.
> >
> > gdb.mi/mi-var-cp.exp?
> > gdb.mi/mi-var-cp.cc?
> >
> > Its for variable objects, and for consistency.  The testsuite has the
> > directory gdb.cp and it's populated with *.cc files
> 
> I've no problems with 'cp' and '.cc'. I don't think that '-var-' is good -- 
> now, there is just single MI test dealing with C++, so I want a testcase that 
> will accumulate all C++ specific things, not necessary related to variable 
> objects. But I don't care much.

I'd rather keep varobj tests easily identifiable; please use Nick's
suggestions.

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI/C++/references fixup

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

> On Wed, Nov 29, 2006 at 05:41:13PM +0300, Vladimir Prus wrote:
>> +# Creates varobj named NAME for EXPRESSION.
>> +# Name cannot be "-".
>> +proc mi_create_varobj { name expression } {
> 
>> +# Updates varobj named NAME and checks that all varobjs in EXPECTED
>> +# are reported as updated, and no other varobj is updated.
>> +# Assumes that no varobj is out of scope and that no varobj changes
>> +# types.
>> +proc mi_varobj_update { name expected } {
> 
>> +proc mi_check_varobj_value { name value } {
> 
> I would recommend giving these an argument for the test name, as some
> of the other helper functions do.  Otherwise, if you call them twice in
> a row with the same arguments (which your test does) then you've got
> two tests with the same name.
> 
> On Thu, Nov 30, 2006 at 10:31:07AM +0300, Vladimir Prus wrote:
>> >         * gdb.mi/mi-cpp.cpp: New file.
>> >
>> > gdb.mi/mi-var-cp.exp?
>> > gdb.mi/mi-var-cp.cc?
>> >
>> > Its for variable objects, and for consistency.  The testsuite has the
>> > directory gdb.cp and it's populated with *.cc files
>> 
>> I've no problems with 'cp' and '.cc'. I don't think that '-var-' is good
>> -- now, there is just single MI test dealing with C++, so I want a
>> testcase that will accumulate all C++ specific things, not necessary
>> related to variable objects. But I don't care much.
> 
> I'd rather keep varobj tests easily identifiable; please use Nick's
> suggestions.

Here's revised patch. Ok for HEAD?

- Volodya

        * varobj.c (varobj_create): Don't call release_value.
        (varobj_set_value): Likewise.
        (install_new_value): Call coerce_ref and release_value
        on the value. Add asserts.

        testsuite/
        * lib/mi-support.exp
        (mi_runto): Accept "()" after function name.
        (mi_create_varobj): New function.
        (mi_varobj_update): New function.
        (mi_Check_varobj_value): New function.
        * gdb.mi/mi-var-cp.exp: New file.
        * gdb.mi/mi-var-cp.cc: New file.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 00REFERENCES.diff --]
[-- Type: text/x-diff; name="00REFERENCES.diff", Size: 4485 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.63
diff -u -p -r1.63 varobj.c
--- varobj.c	6 Dec 2006 09:01:50 -0000	1.63
+++ varobj.c	6 Dec 2006 09:12:26 -0000
@@ -512,10 +512,7 @@ varobj_create (char *objname,
 	   right type.  */
 	value = evaluate_type (var->root->exp);
 
-      release_value (value);
-
       var->type = value_type (value);
-
       install_new_value (var, value, 1 /* Initial assignment */);
 
       /* Set language info */
@@ -847,9 +844,7 @@ varobj_set_value (struct varobj *var, ch
 	 with catch the exception.  */
       if (!gdb_value_assign (var->value, value, &val))
 	return 0;
-
-      release_value (val);
-      
+     
       /* If the value has changed, record it, so that next -var-update can
 	 report this change.  If a variable had a value of '1', we've set it
 	 to '333' and then set again to '1', when -var-update will report this
@@ -901,7 +896,10 @@ varobj_list (struct varobj ***varlist)
    and return 0.
    Otherwise, assign the value and if type_changeable returns non-zero,
    find if the new value is different from the current value.
-   Return 1 if so, and 0 if the values are equal.  */
+   Return 1 if so, and 0 if the values are equal.  
+
+   The VALUE parameter should not be released -- the function will
+   take care of releasing it when needed.  */
 static int
 install_new_value (struct varobj *var, struct value *value, int initial)
 { 
@@ -917,6 +915,15 @@ install_new_value (struct varobj *var, s
   changeable = type_changeable (var);
   need_to_fetch = changeable;
 
+  /* We are not interested in the address of references, and given
+     that in C++ a reference is not rebindable, it cannot
+     meaningfully change.  So, get hold of the real value.  */
+  if (value)
+    {
+      value = coerce_ref (value);
+      release_value (value);
+    }
+
   if (var->type && TYPE_CODE (var->type) == TYPE_CODE_UNION)
     /* For unions, we need to fetch the value implicitly because
        of implementation of union member fetch.  When gdb
@@ -985,6 +992,8 @@ install_new_value (struct varobj *var, s
     value_free (var->value);
   var->value = value;
   var->updated = 0;
+  
+  gdb_assert (!var->value || value_type (var->value));
 
   return changed;
 }
Index: testsuite/lib/mi-support.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/mi-support.exp,v
retrieving revision 1.33
diff -u -p -r1.33 mi-support.exp
--- testsuite/lib/mi-support.exp	24 Jul 2006 20:38:08 -0000	1.33
+++ testsuite/lib/mi-support.exp	6 Dec 2006 09:12:27 -0000
@@ -869,7 +869,7 @@ proc mi_runto {func} {
 
   set test "mi runto $func"
   mi_gdb_test "200-break-insert $func" \
-    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
+    "200\\^done,bkpt=\{number=\"\[0-9\]+\",type=\"breakpoint\",disp=\"keep\",enabled=\"y\",addr=\"$hex\",func=\"$func\(\\\(.*\\\)\)?\",file=\".*\",line=\"\[0-9\]*\",times=\"0\"\}" \
     "breakpoint at $func"
 
   if {![regexp {number="[0-9]+"} $expect_out(buffer) str]
@@ -998,3 +998,39 @@ proc mi0_continue_to { bkptno func args 
     mi0_execute_to "exec-continue" "breakpoint-hit\",bkptno=\"$bkptno" \
 	"$func" "$args" "$file" "$line" "" "$test"
 }
+
+# Creates varobj named NAME for EXPRESSION.
+# Name cannot be "-".
+proc mi_create_varobj { name expression testname } {
+    mi_gdb_test "-var-create $name * $expression" \
+        "\\^done,name=\"$name\",numchild=\"\[0-9\]+\",type=.*" \
+        $testname
+}
+
+# Updates varobj named NAME and checks that all varobjs in EXPECTED
+# are reported as updated, and no other varobj is updated.
+# Assumes that no varobj is out of scope and that no varobj changes
+# types.
+proc mi_varobj_update { name expected testname } {
+    set er "\\^done,changelist=\\\["
+    set first 1
+    foreach item $expected {
+        set v "{name=\"$item\",in_scope=\"true\",type_changed=\"false\"}"
+        if {$first} {
+            set er "$er$v"
+        } else {
+            set er "$er,$v"
+        }
+    }
+    set er "$er\\\]"
+
+    verbose -log "Expecting: $er" 2
+    mi_gdb_test "-var-update $name" $er $testname
+}
+
+proc mi_check_varobj_value { name value testname } {
+
+    mi_gdb_test "-var-evaluate-expression $name" \
+	"\\^done,value=\"$value\"" \
+	$testname
+}

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: mi-var-cp.cc --]
[-- Type: text/x-objcsrc; name="mi-var-cp.cc", Size: 915 bytes --]

/* 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.  
*/

void reference_update_tests ()
{
  int x = 167;
  int& rx = x;
  x = 567;
  x = 567;
}

int main ()
{
  reference_update_tests ();
  return 0;
}

[-- Attachment #4: mi-var-cp.exp --]
[-- Type: text/plain, Size: 2133 bytes --]

# 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.  

if { [skip_cplus_tests] } { continue }

load_lib mi-support.exp
set MIFLAGS "-i=mi"

gdb_exit
if [mi_gdb_start] {
    continue
}

set testfile mi-var-cp
set srcfile "$testfile.cc"
set binfile $objdir/$subdir/$testfile

if [get_compiler_info ${binfile} "c++"] {
    return -1;
}

if {[gdb_compile $srcdir/$subdir/$srcfile $binfile executable {debug c++}] != ""} {
  untested $testfile.exp
  return -1
}

mi_gdb_load ${binfile}

# Test that children of classes are properly reported

# Run to main
mi_runto reference_update_tests

mi_create_varobj "RX" "rx" "create varobj for rx"

set x_assignment [gdb_get_line_number "x = 567;"]
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment-1] \
    "step to x assignment"
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment] \
    "step to x assignment"

mi_varobj_update RX {RX} "update RX (1)"

mi_check_varobj_value RX 167 "check RX: expect 167"

# Execute the first 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+1] \
    "step to x assignment"

mi_varobj_update RX {RX} "update RX (2)"
mi_check_varobj_value RX 567 "check RX: expect 567"

# Execute the second 'x = 567' line.
mi_next_to "reference_update_tests" {} ".*${srcfile}" [expr $x_assignment+2] \
    "step to x assignment"

mi_varobj_update RX {} "update RX (3)"

mi_gdb_exit
return 0

Re: MI/C++/references fixup

[Daniel Jacobowitz]

On Wed, Dec 06, 2006 at 12:16:45PM +0300, Vladimir Prus wrote:
> Here's revised patch. Ok for HEAD?
> 
> - Volodya
> 
>         * varobj.c (varobj_create): Don't call release_value.
>         (varobj_set_value): Likewise.
>         (install_new_value): Call coerce_ref and release_value
>         on the value. Add asserts.
> 
>         testsuite/
>         * lib/mi-support.exp
>         (mi_runto): Accept "()" after function name.
>         (mi_create_varobj): New function.
>         (mi_varobj_update): New function.
>         (mi_Check_varobj_value): New function.
>         * gdb.mi/mi-var-cp.exp: New file.
>         * gdb.mi/mi-var-cp.cc: New file.

Yes, this is OK.  Thanks!

-- 
Daniel Jacobowitz
CodeSourcery

Re: MI/C++/references fixup

[Vladimir Prus]

Daniel Jacobowitz wrote:

> On Wed, Dec 06, 2006 at 12:16:45PM +0300, Vladimir Prus wrote:
>> Here's revised patch. Ok for HEAD?
>> 
>> - Volodya
>> 
>>         * varobj.c (varobj_create): Don't call release_value.
>>         (varobj_set_value): Likewise.
>>         (install_new_value): Call coerce_ref and release_value
>>         on the value. Add asserts.
>> 
>>         testsuite/
>>         * lib/mi-support.exp
>>         (mi_runto): Accept "()" after function name.
>>         (mi_create_varobj): New function.
>>         (mi_varobj_update): New function.
>>         (mi_Check_varobj_value): New function.
>>         * gdb.mi/mi-var-cp.exp: New file.
>>         * gdb.mi/mi-var-cp.cc: New file.
> 
> Yes, this is OK.  Thanks!

Thanks, checked in.

- Volodya