Fix crash in -var-delete

Fix crash in -var-delete

[Vladimir Prus]

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


A KDevelop user has reported that gdb 6.7 consistently crashes when
used in KDevelop. Of course, I should have tested gdb 6.7 before it's released,
but it's too late at this point, so here's a patch instead.

Historically, KDevelop deletes variable objects recursively -- first most
nested ones, then their parents and so on. Note that while it's not necessary
in most cases, there's nothing inherently wrong about it. Say,  if
GUI for some reason does not care about particular child anymore, it's free to
delete it (even though with frozen varobj, it can just freeze it). 

Unfortunately gdb 6.7, when deleting varobj where some children of it
are already deleted, tries to mess with NULL pointers, and crashes.

The patch and a testcase are attached. OK?

- Volodya




[-- Attachment #2: child_deletion.diff --]
[-- Type: text/x-diff, Size: 2430 bytes --]

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.96
diff -u -p -r1.96 varobj.c
--- varobj.c	27 Sep 2007 18:04:12 -0000	1.96
+++ varobj.c	7 Nov 2007 19:51:19 -0000
@@ -1292,6 +1292,8 @@ delete_variable_1 (struct cpstack **resu
   for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
     {   
       varobj_p child = VEC_index (varobj_p, var->children, i);
+      if (!child)
+	continue;
       if (!remove_from_parent_p)
 	child->parent = NULL;
       delete_variable_1 (resultp, delcountp, child, 0, only_children_p);
Index: testsuite/gdb.mi/mi-var-child.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-child.c,v
retrieving revision 1.6
diff -u -p -r1.6 mi-var-child.c
--- testsuite/gdb.mi/mi-var-child.c	23 Aug 2007 18:08:49 -0000	1.6
+++ testsuite/gdb.mi/mi-var-child.c	7 Nov 2007 19:51:20 -0000
@@ -306,6 +306,29 @@ do_special_tests (void)
   incr_a(2);
 }
 
+struct very_simple_struct
+{
+  int a;
+  int b;
+};
+
+int
+do_child_deletion (void)
+{
+  /*: BEGIN: child_deletion :*/
+  struct very_simple_struct s = {1, 2};
+  /*:
+    mi_create_varobj S s "create varobj for s"
+    mi_list_varobj_children S {{S.a a 0 int} {S.b b 0 int}}	\
+       "list children of S"
+    mi_delete_varobj S.a "delete S.a"    
+    mi_delete_varobj S.b "delete S.b"
+    mi_delete_varobj S "delete S"
+    :*/
+  return 99;
+  /*: END: child_deletion :*/  
+}
+
 int
 main (int argc, char *argv [])
 {
@@ -313,6 +336,7 @@ main (int argc, char *argv [])
   do_block_tests ();
   do_children_tests ();
   do_special_tests ();
+  do_child_deletion ();
   exit (0);
 }
 
Index: testsuite/gdb.mi/mi-var-child.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-child.exp,v
retrieving revision 1.26
diff -u -p -r1.26 mi-var-child.exp
--- testsuite/gdb.mi/mi-var-child.exp	23 Aug 2007 18:14:19 -0000	1.26
+++ testsuite/gdb.mi/mi-var-child.exp	7 Nov 2007 19:51:20 -0000
@@ -1227,7 +1227,9 @@ mi_gdb_test "-var-update *" \
 	"\\^done,changelist=\\\[\{name=\"psnp->ptrs.0.next.next.long_ptr\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
 	"update all vars psnp->next->next->long_ptr (and 2.long_ptr) changed"
 
+mi_prepare_inline_tests $srcfile
 
+mi_run_inline_test child_deletion
 
 
 mi_gdb_exit

Re: Fix crash in -var-delete

[Daniel Jacobowitz]

On Wed, Nov 07, 2007 at 10:57:59PM +0300, Vladimir Prus wrote:
> The patch and a testcase are attached. OK?

OK (if you write the changelog)

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix crash in -var-delete

[Vladimir Prus]

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

Daniel Jacobowitz wrote:

> On Wed, Nov 07, 2007 at 10:57:59PM +0300, Vladimir Prus wrote:
>> The patch and a testcase are attached. OK?
> 
> OK (if you write the changelog)

Thanks. Here's what I've checked in (with changelog).

- Volodya



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

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.8910
diff -u -p -r1.8910 ChangeLog
--- ChangeLog	7 Nov 2007 06:58:31 -0000	1.8910
+++ ChangeLog	7 Nov 2007 20:05:31 -0000
@@ -1,3 +1,11 @@
+2007-11-07  Vladimir Prus  <vladimir@codesourcery.com>
+
+	Fix crash when a variable object being deleted
+	has any of its children deleted previously.
+	
+	* varobj.c (delete_variable_1): Don't recurse
+	into deleted children.
+	
 2007-11-07  Markus Deuling  <deuling@de.ibm.com>
 
 	* gdbarch.sh (legacy_virtual_frame_pointer): Add gdbarch parameter.
Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.96
diff -u -p -r1.96 varobj.c
--- varobj.c	27 Sep 2007 18:04:12 -0000	1.96
+++ varobj.c	7 Nov 2007 20:05:32 -0000
@@ -1292,6 +1292,8 @@ delete_variable_1 (struct cpstack **resu
   for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
     {   
       varobj_p child = VEC_index (varobj_p, var->children, i);
+      if (!child)
+	continue;
       if (!remove_from_parent_p)
 	child->parent = NULL;
       delete_variable_1 (resultp, delcountp, child, 0, only_children_p);
Index: testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.1483
diff -u -p -r1.1483 ChangeLog
--- testsuite/ChangeLog	5 Nov 2007 11:32:31 -0000	1.1483
+++ testsuite/ChangeLog	7 Nov 2007 20:05:37 -0000
@@ -1,3 +1,8 @@
+2007-11-07  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* gdb.mi/mi-var-child.c (do_child_deletion): New.
+	* gdb.mi/mi-var-child.exp: Run child_deletion tests.
+	
 2007-11-05  Luis Machado  <luisgpm@br.ibm.com>
 
 	* gdb.base/printcmds.exp: New function
Index: testsuite/gdb.mi/mi-var-child.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-child.c,v
retrieving revision 1.6
diff -u -p -r1.6 mi-var-child.c
--- testsuite/gdb.mi/mi-var-child.c	23 Aug 2007 18:08:49 -0000	1.6
+++ testsuite/gdb.mi/mi-var-child.c	7 Nov 2007 20:05:37 -0000
@@ -306,6 +306,29 @@ do_special_tests (void)
   incr_a(2);
 }
 
+struct very_simple_struct
+{
+  int a;
+  int b;
+};
+
+int
+do_child_deletion (void)
+{
+  /*: BEGIN: child_deletion :*/
+  struct very_simple_struct s = {1, 2};
+  /*:
+    mi_create_varobj S s "create varobj for s"
+    mi_list_varobj_children S {{S.a a 0 int} {S.b b 0 int}}	\
+       "list children of S"
+    mi_delete_varobj S.a "delete S.a"    
+    mi_delete_varobj S.b "delete S.b"
+    mi_delete_varobj S "delete S"
+    :*/
+  return 99;
+  /*: END: child_deletion :*/  
+}
+
 int
 main (int argc, char *argv [])
 {
@@ -313,6 +336,7 @@ main (int argc, char *argv [])
   do_block_tests ();
   do_children_tests ();
   do_special_tests ();
+  do_child_deletion ();
   exit (0);
 }
 
Index: testsuite/gdb.mi/mi-var-child.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-child.exp,v
retrieving revision 1.26
diff -u -p -r1.26 mi-var-child.exp
--- testsuite/gdb.mi/mi-var-child.exp	23 Aug 2007 18:14:19 -0000	1.26
+++ testsuite/gdb.mi/mi-var-child.exp	7 Nov 2007 20:05:37 -0000
@@ -1227,7 +1227,9 @@ mi_gdb_test "-var-update *" \
 	"\\^done,changelist=\\\[\{name=\"psnp->ptrs.0.next.next.long_ptr\",in_scope=\"true\",type_changed=\"false\"\}\\\]" \
 	"update all vars psnp->next->next->long_ptr (and 2.long_ptr) changed"
 
+mi_prepare_inline_tests $srcfile
 
+mi_run_inline_test child_deletion
 
 
 mi_gdb_exit

Re: Fix crash in -var-delete

[Joel Brobecker]

> > On Wed, Nov 07, 2007 at 10:57:59PM +0300, Vladimir Prus wrote:
> >> The patch and a testcase are attached. OK?
> > 
> > OK (if you write the changelog)
> 
> Thanks. Here's what I've checked in (with changelog).

Can you also please commit to the branch?

I suggest that we add a NEWS entry as well for every fixes that we make
on the branch, that really helps my work when writing the announcement
of a new release.

Thanks a lot,
-- 
Joel

Re: Fix crash in -var-delete

[Nick Roberts]

 > @@ -1292,6 +1292,8 @@ delete_variable_1 (struct cpstack **resu
 >    for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
 >      {   
 >        varobj_p child = VEC_index (varobj_p, var->children, i);
 > +      if (!child)
 > +	continue;
 >        if (!remove_from_parent_p)
 >  	child->parent = NULL;

Should child not get removed from the list of its parents' children when
deleted first time round?  Then checking for child shouldn't be necessary.

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

Re: Fix crash in -var-delete

[Vladimir Prus]

On Thursday 08 November 2007 08:27:14 Nick Roberts wrote:
>  > @@ -1292,6 +1292,8 @@ delete_variable_1 (struct cpstack **resu
>  >    for (i = 0; i < VEC_length (varobj_p, var->children); ++i)
>  >      {   
>  >        varobj_p child = VEC_index (varobj_p, var->children, i);
>  > +      if (!child)
>  > +	continue;
>  >        if (!remove_from_parent_p)
>  >  	child->parent = NULL;
> 
> Should child not get removed from the list of its parents' children when
> deleted first time round?  Then checking for child shouldn't be necessary.

This would have worked previously, when children of varobj were in a linked
list, and search was done by field name. However, that approach did not
work when a field of structure had no name, which is quite possible in C and
C++. So how, children of a varobj are stored as a vector. Each child knows
its index in parent, and when a child is removed the corresponding entry
in parent's vector is set to NULL. I presume it's possible to store list
of children as vector of (index,child) pair, which would allow removing
a child completely, but the complexity involved in that will outweight
the complexity of checking for NULL child here.

- Volodya

Re: Fix crash in -var-delete

[Vladimir Prus]

On Wednesday 07 November 2007 23:55:02 Joel Brobecker wrote:
> > > On Wed, Nov 07, 2007 at 10:57:59PM +0300, Vladimir Prus wrote:
> > >> The patch and a testcase are attached. OK?
> > > 
> > > OK (if you write the changelog)
> > 
> > Thanks. Here's what I've checked in (with changelog).
> 
> Can you also please commit to the branch?

Done.

> I suggest that we add a NEWS entry as well for every fixes that we make
> on the branch, that really helps my work when writing the announcement
> of a new release.

Can you clarify if you want a NEWS entry to be checked in on trunk, on
branch, or both?

- Volodya

Re: Fix crash in -var-delete

[Joel Brobecker]

> > Can you also please commit to the branch?
> 
> Done.

Thank you.

> > I suggest that we add a NEWS entry as well for every fixes that we make
> > on the branch, that really helps my work when writing the announcement
> > of a new release.
> 
> Can you clarify if you want a NEWS entry to be checked in on trunk, on
> branch, or both?

Just the branch. This should be an RFA because it's a documentation
change, and it is not obvious that this change will be accepted.
I don't think this is a diversion of the purpose of the NEWS files,
but others may disagree.

Thank you,
-- 
Joel