[mi] Some error path fixes

[mi] Some error path fixes

[Daniel Jacobowitz]

Two patches I've had locally that fix up problems reading variables.
I couldn't find a way to write a test for the eval.c part; I wrote this
a long time ago, when we called error for variables that were currently
unavailable, at which time it was much easier to trigger the crash.  The
varobj.c part is covered by the new tests.  gdb_evaluate_expression
does not set *value = NULL on error.

(runtest --tool_exec "valgrind ../gdb" works reasonably well, though you
have to inspect the logs by hand.)

Does this look wrong to anyone?

I think there's still something suspicious about the error handling.  If you
take a look at the output from the "update endvar" test, you'll never get an
in_scope="false", which I would have expected for a variable that couldn't
be updated.  That's because all is well until install_new_value. If we go
from var->error = 0 to var->error = 1, at least, shouldn't we report that?

-- 
Daniel Jacobowitz
CodeSourcery

2006-12-07  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (eval.o): Update dependencies.
	* eval.c: Include "ui-out.h" and "exceptions.h".
	(evaluate_subexp_standard): Use TRY_CATCH around value_of_variable.
	Use value_zero if an error occurs when avoiding side effects.
	* varobj.c (c_value_of_root): Initialize new_val.

2006-12-07  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.mi/mi-var-cmd.exp: Add tests for unreadable varobjs.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.856
diff -u -p -r1.856 Makefile.in
--- Makefile.in	28 Nov 2006 22:14:31 -0000	1.856
+++ Makefile.in	7 Dec 2006 21:54:55 -0000
@@ -1975,7 +1975,7 @@ environ.o: environ.c $(defs_h) $(environ
 eval.o: eval.c $(defs_h) $(gdb_string_h) $(symtab_h) $(gdbtypes_h) \
 	$(value_h) $(expression_h) $(target_h) $(frame_h) $(language_h) \
 	$(f_lang_h) $(cp_abi_h) $(infcall_h) $(objc_lang_h) $(block_h) \
-	$(parser_defs_h) $(cp_support_h)
+	$(parser_defs_h) $(cp_support_h) $(exceptions_h) $(uiout_h)
 event-loop.o: event-loop.c $(defs_h) $(event_loop_h) $(event_top_h) \
 	$(gdb_string_h) $(exceptions_h) $(gdb_assert_h) $(gdb_select_h)
 event-top.o: event-top.c $(defs_h) $(top_h) $(inferior_h) $(target_h) \
Index: eval.c
===================================================================
RCS file: /cvs/src/src/gdb/eval.c,v
retrieving revision 1.64
diff -u -p -r1.64 eval.c
--- eval.c	10 Oct 2006 03:17:53 -0000	1.64
+++ eval.c	7 Dec 2006 21:54:55 -0000
@@ -37,6 +37,8 @@
 #include "block.h"
 #include "parser-defs.h"
 #include "cp-support.h"
+#include "ui-out.h"
+#include "exceptions.h"
 
 /* This is defined in valops.c */
 extern int overload_resolution;
@@ -465,8 +467,26 @@ evaluate_subexp_standard (struct type *e
 	 value_rtti_target_type () if we are dealing with a pointer
 	 or reference to a base class and print object is on. */
 
-	return value_of_variable (exp->elts[pc + 2].symbol,
-				  exp->elts[pc + 1].block);
+      {
+	volatile struct gdb_exception except;
+	struct value *ret = NULL;
+
+	TRY_CATCH (except, RETURN_MASK_ERROR)
+	  {
+	    ret = value_of_variable (exp->elts[pc + 2].symbol,
+				     exp->elts[pc + 1].block);
+	  }
+
+	if (except.reason < 0)
+	  {
+	    if (noside == EVAL_AVOID_SIDE_EFFECTS)
+	      ret = value_zero (SYMBOL_TYPE (exp->elts[pc + 2].symbol), not_lval);
+	    else
+	      throw_exception (except);
+	  }
+
+	return ret;
+      }
 
     case OP_LAST:
       (*pos) += 2;
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	7 Dec 2006 21:54:55 -0000
@@ -1928,7 +1928,7 @@ c_name_of_child (struct varobj *parent, 
 static struct value *
 c_value_of_root (struct varobj **var_handle)
 {
-  struct value *new_val;
+  struct value *new_val = NULL;
   struct varobj *var = *var_handle;
   struct frame_info *fi;
   int within_scope;
Index: testsuite/gdb.mi/mi-var-cmd.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-cmd.exp,v
retrieving revision 1.20
diff -u -p -r1.20 mi-var-cmd.exp
--- testsuite/gdb.mi/mi-var-cmd.exp	28 Nov 2006 17:23:10 -0000	1.20
+++ testsuite/gdb.mi/mi-var-cmd.exp	7 Dec 2006 21:54:55 -0000
@@ -599,5 +599,48 @@ mi_gdb_test "-var-update selected_a" \
 	"\\^done,changelist=\\\[\{name=\"selected_a\",in_scope=\"true\",new_type=\"int\",new_num_children=\"0\"\}\\\]" \
 	"update selected_a in do_special_tests"
 
+# Test whether bad varobjs crash GDB.
+
+# A varobj we fail to read during -var-update should be considered
+# out of scope.
+mi_gdb_test "-var-create null_ptr * **0" \
+    {\^done,name="null_ptr",numchild="0",type="int"} \
+    "create null_ptr"
+
+# Allow this to succeed, if address zero is readable, although it
+# will not test what it was meant to.  Most important is that GDB
+# does not crash.
+mi_gdb_test "-var-update null_ptr" \
+    {\^done,changelist=\[{.*}\]} \
+    "update null_ptr"
+
+mi_gdb_test "-var-delete null_ptr" \
+    "\\^done,ndeleted=\"1\"" \
+    "delete null_ptr"
+
+# When we fail to read a varobj created from a named variable,
+# we evaluate its type instead.  Make sure that doesn't blow
+# up by trying to read it again.  We can use _end when not running
+# the program to simulate an unreadable variable, if this platform
+# provides _end, but cope if it's missing.
+
+mi_gdb_test "kill" \
+    {&"kill\\n".*\^done} \
+    "kill program before endvar"
+
+mi_gdb_test "-var-create endvar * _end" \
+    {(\^done,name="endvar",numchild="0",type=".*"|&".*unable to.*".*\^error,msg=".*")} \
+    "create endvar"
+
+# Allow this to succeed whether the value is readable, unreadable, or
+# missing.  Most important is that GDB does not crash.
+mi_gdb_test "-var-update endvar" \
+    {(\^done,changelist=\[.*\]|^".*".*\^error,msg=".*not found")} \
+    "update endvar"
+
+mi_gdb_test "-var-delete endvar" \
+    "\\^done,ndeleted=\"1\"" \
+    "delete endvar"
+
 mi_gdb_exit
 return 0

Re: [mi] Some error path fixes

[Daniel Jacobowitz]

On Thu, Dec 07, 2006 at 05:00:13PM -0500, Daniel Jacobowitz wrote:
> 2006-12-07  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* Makefile.in (eval.o): Update dependencies.
> 	* eval.c: Include "ui-out.h" and "exceptions.h".
> 	(evaluate_subexp_standard): Use TRY_CATCH around value_of_variable.
> 	Use value_zero if an error occurs when avoiding side effects.
> 	* varobj.c (c_value_of_root): Initialize new_val.
> 
> 2006-12-07  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* gdb.mi/mi-var-cmd.exp: Add tests for unreadable varobjs.

I retested this and checked it in.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [mi] Some error path fixes

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> 2006-12-07  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* gdb.mi/mi-var-cmd.exp: Add tests for unreadable varobjs.

> +# Test whether bad varobjs crash GDB.
> +
> +# A varobj we fail to read during -var-update should be considered
> +# out of scope.
> +mi_gdb_test "-var-create null_ptr * **0" \
> +    {\^done,name="null_ptr",numchild="0",type="int"} \
> +    "create null_ptr"
> +
> +# Allow this to succeed, if address zero is readable, although it
> +# will not test what it was meant to.  Most important is that GDB
> +# does not crash.
> +mi_gdb_test "-var-update null_ptr" \
> +    {\^done,changelist=\[{.*}\]} \
> +    "update null_ptr"

This fails on SPU, where address zero is indeed readable.  I get:

-var-create null_ptr * **0^M
^done,name="null_ptr",numchild="0",value="",type="int"^M
(gdb) ^M
PASS: gdb.mi/mi-var-cmd.exp: create null_ptr
-var-update null_ptr^M
^done,changelist=[]^M
(gdb) ^M
FAIL: gdb.mi/mi-var-cmd.exp: update null_ptr

Note the completely empty changelist, while the test checks
for at least one element { ... }.

Is this a testcase bug, or should there really be a change?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [mi] Some error path fixes

[Daniel Jacobowitz]

I'm really not showing well today... :-(

On Fri, Mar 09, 2007 at 01:55:20AM +0100, Ulrich Weigand wrote:
> This fails on SPU, where address zero is indeed readable.  I get:
> 
> -var-create null_ptr * **0^M
> ^done,name="null_ptr",numchild="0",value="",type="int"^M
> (gdb) ^M
> PASS: gdb.mi/mi-var-cmd.exp: create null_ptr
> -var-update null_ptr^M
> ^done,changelist=[]^M
> (gdb) ^M
> FAIL: gdb.mi/mi-var-cmd.exp: update null_ptr
> 
> Note the completely empty changelist, while the test checks
> for at least one element { ... }.
> 
> Is this a testcase bug, or should there really be a change?

I think you're right - the braces should be removed.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [mi] Some error path fixes

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I think you're right - the braces should be removed.

OK, I've committed the following patch, which fixes the
test case on spu-elf.  Thanks!

Bye,
Ulrich


ChangeLog:

	* gdb.mi/mi-var-cmd.exp: Allow -var-update null_ptr test to
	pass on targets where address zero is readable.

Index: gdb/testsuite/gdb.mi/mi-var-cmd.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.mi/mi-var-cmd.exp,v
retrieving revision 1.27
diff -u -p -r1.27 mi-var-cmd.exp
--- gdb/testsuite/gdb.mi/mi-var-cmd.exp	28 Feb 2007 17:32:51 -0000	1.27
+++ gdb/testsuite/gdb.mi/mi-var-cmd.exp	9 Mar 2007 01:11:37 -0000
@@ -603,7 +603,7 @@ mi_gdb_test "-var-create null_ptr * **0"
 # will not test what it was meant to.  Most important is that GDB
 # does not crash.
 mi_gdb_test "-var-update null_ptr" \
-    {\^done,changelist=\[{.*}\]} \
+    {\^done,changelist=\[.*\]} \
     "update null_ptr"
 
 mi_gdb_test "-var-delete null_ptr" \


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com