[PATCH] Catch errors in value_get_print_value

[PATCH] Catch errors in value_get_print_value

[Nick Roberts]

Currently when common_val_print throws an error from install_new_value it gets
caught in mi_execute_command and prints an MI error (^error,...)  without
cleaning up.  This patch catches the error in value_get_print_value so that GDB
can proceed normally.  It could be adapted to output something like
"<error_reading_variable>" but I think the empty string works better.

To see what currently happens, compile:

#include <stdlib.h>

typedef struct {
  float a;
  float b;
} substruct;

struct substruct1 {
  float a1;
  substruct b1;
};

main () {
  
struct substruct1 *abc4 = NULL;
 int i = 1;
}

-------------------
and run under GDB as follows:


~"Using host libthread_db library \"/lib/libthread_db.so.1\".\n"
(gdb)
start
&"start\n"
Breakpoint 1 at 0x8048365: file temp4.c, line 15.
Starting program: /home/nickrob/temp4
main () at temp4.c:15
15      struct substruct1 *abc4 = NULL;
^done
(gdb)
n
&"n\n"
16       int i = 1;
^done
(gdb)
-var-create - * abc4
^done,name="var1",numchild="2",type="struct substruct1 *"
(gdb)
-var-list-children --all-values var1
&"Cannot access memory at address 0x4\n"
^error,msg="Cannot access memory at address 0x4"
(gdb)
-var-list-children --all-values var1
&&"Duplicate variable object name\n"
^error,msg="Duplicate variable object name"
(gdb)


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


2007-02-02  Nick Roberts  <nickrob@snap.net.nz>

	* varobj.c (value_get_print_value): Catch errors from
	common_val_print.


  
*** varobj.c	02 Feb 2007 09:32:33 +1300	1.82
--- varobj.c	02 Feb 2007 09:48:59 +1300	
*************** value_get_print_value (struct value *val
*** 1702,1707 ****
--- 1702,1708 ----
    struct ui_file *stb;
    struct cleanup *old_chain;
    char *thevalue;
+   struct gdb_exception except;
  
    if (value == NULL)
      return NULL;
*************** value_get_print_value (struct value *val
*** 1709,1715 ****
    stb = mem_fileopen ();
    old_chain = make_cleanup_ui_file_delete (stb);
  
!   common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
    thevalue = ui_file_xstrdup (stb, &dummy);
  
    do_cleanups (old_chain);
--- 1710,1720 ----
    stb = mem_fileopen ();
    old_chain = make_cleanup_ui_file_delete (stb);
  
!   TRY_CATCH (except, RETURN_MASK_ERROR)
!     {
!       common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
!     }
! 
    thevalue = ui_file_xstrdup (stb, &dummy);
  
    do_cleanups (old_chain);

Re: [PATCH] Catch errors in value_get_print_value

[Daniel Jacobowitz]

On Fri, Feb 02, 2007 at 09:51:39AM +1300, Nick Roberts wrote:
> 
> Currently when common_val_print throws an error from install_new_value it gets
> caught in mi_execute_command and prints an MI error (^error,...)  without
> cleaning up.  This patch catches the error in value_get_print_value so that GDB
> can proceed normally.  It could be adapted to output something like
> "<error_reading_variable>" but I think the empty string works better.

Could you explain why?  I would be confused to see an empty value in
my locals window.  That's separate from this bug though.

> !   TRY_CATCH (except, RETURN_MASK_ERROR)
> !     {
> !       common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
> !     }
> ! 

val_print, called by common_val_print, already has code to catch this
error and prettyprint it.  It doesn't work in this case, through
common_val_print, because of:

#2  0x0000000000445abd in memory_error (status=5, memaddr=4)
    at /space/fsf/commit/src/gdb/corefile.c:229
#3  0x00000000004aac7f in value_fetch_lazy (val=0x9688c0)
    at /space/fsf/commit/src/gdb/valops.c:549
#4  0x00000000004a2f2f in value_contents_all (value=0x9688c0)
    at /space/fsf/commit/src/gdb/value.c:331
#5  0x00000000004b19a4 in common_val_print (val=0x9688c0,
    stream=0x94a6b0, format=0, deref_ref=1, recurse=0,
    pretty=Val_no_prettyprint)
    at /space/fsf/commit/src/gdb/valprint.c:284

I tried handling the error in value_check_printable.  That doesn't have
quite the results that I wanted, because if install_new_value is called
first it sets value = NULL instead of leaving the unfetchable value
there.  That's what happens for a1.

That gave me an idea.  The error only arises for b1 in your testcase,
where initial && !changeable and indeed we never use var->print_value
if !changeable.  So avoiding common_val_print in that case avoids the
error.

So I checked this in instead.  Let me know if it doesn't work for you.

-- 
Daniel Jacobowitz
CodeSourcery

2007-02-08  Daniel Jacobowitz  <dan@codesourcery.com>

	* varobj.c (install_new_value): Only call value_get_print_value
	if changeable.

Index: varobj.c
===================================================================
RCS file: /cvs/src/src/gdb/varobj.c,v
retrieving revision 1.82
diff -u -p -r1.82 varobj.c
--- varobj.c	24 Jan 2007 19:54:13 -0000	1.82
+++ varobj.c	8 Feb 2007 17:38:36 -0000
@@ -953,7 +953,7 @@ install_new_value (struct varobj *var, s
   /* If the type is changeable, compare the old and the new values.
      If this is the initial assignment, we don't have any old value
      to compare with.  */
-  if (initial)
+  if (initial && changeable)
     var->print_value = value_get_print_value (value, var->format);
   else if (changeable)
     {

Re: [PATCH] Catch errors in value_get_print_value

[Nick Roberts]

 > > Currently when common_val_print throws an error from install_new_value it gets
 > > caught in mi_execute_command and prints an MI error (^error,...)  without
 > > cleaning up.  This patch catches the error in value_get_print_value so that GDB
 > > can proceed normally.  It could be adapted to output something like
 > > "<error_reading_variable>" but I think the empty string works better.
 > 
 > Could you explain why?  I would be confused to see an empty value in
 > my locals window.  That's separate from this bug though.

It's much quieter than a window full of error/warning messages.  If there's no
value available, the frontend doesn't show one.  That seems logical to me but
perhaps only because I'm used to it.


 > > !   TRY_CATCH (except, RETURN_MASK_ERROR)
 > > !     {
 > > !       common_val_print (value, stb, format_code[(int) format], 1, 0, 0);
 > > !     }
 > > ! 
 >
 > val_print, called by common_val_print, already has code to catch this
 > error and prettyprint it.  

Yes but I think the error is in value_contents_all.  It's the only way that I can
catch it in varobj.c.

 ...
 > So I checked this in instead.  Let me know if it doesn't work for you.
 >
 > -- 
 > Daniel Jacobowitz
 > CodeSourcery
 > 
 > 2007-02-08  Daniel Jacobowitz  <dan@codesourcery.com>
 > 
 > 	* varobj.c (install_new_value): Only call value_get_print_value
 > 	if changeable.
 > 
 > Index: varobj.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/varobj.c,v
 > retrieving revision 1.82
 > diff -u -p -r1.82 varobj.c
 > --- varobj.c	24 Jan 2007 19:54:13 -0000	1.82
 > +++ varobj.c	8 Feb 2007 17:38:36 -0000
 > @@ -953,7 +953,7 @@ install_new_value (struct varobj *var, s
 >    /* If the type is changeable, compare the old and the new values.
 >       If this is the initial assignment, we don't have any old value
 >       to compare with.  */
 > -  if (initial)
 > +  if (initial && changeable)
 >      var->print_value = value_get_print_value (value, var->format);
 >    else if (changeable)
 >      {

Thanks.  This is clearly a better patch as it's much simpler.  Thinking more
generally, I wonder if there are other cases where value_contents_all should
catch this error?

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

Re: [PATCH] Catch errors in value_get_print_value

[Daniel Jacobowitz]

On Fri, Feb 09, 2007 at 11:18:52AM +1300, Nick Roberts wrote:
>  > > Currently when common_val_print throws an error from install_new_value it gets
>  > > caught in mi_execute_command and prints an MI error (^error,...)  without
>  > > cleaning up.  This patch catches the error in value_get_print_value so that GDB
>  > > can proceed normally.  It could be adapted to output something like
>  > > "<error_reading_variable>" but I think the empty string works better.
>  > 
>  > Could you explain why?  I would be confused to see an empty value in
>  > my locals window.  That's separate from this bug though.
> 
> It's much quieter than a window full of error/warning messages.  If there's no
> value available, the frontend doesn't show one.  That seems logical to me but
> perhaps only because I'm used to it.

Well, maybe we could use in_scope="false" and no value, and let the
front end decide...

> Thanks.  This is clearly a better patch as it's much simpler.  Thinking more
> generally, I wonder if there are other cases where value_contents_all should
> catch this error?

I don't think so - would there be a sensible return value for
value_contents_all if something has gone wrong?  Not really, so I think
it's the responsibility of other layers to detect it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Catch errors in value_get_print_value

[Nick Roberts]

 > > It's much quieter than a window full of error/warning messages.  If
 > > there's no value available, the frontend doesn't show one.  That seems
 > > logical to me but perhaps only because I'm used to it.
 > 
 > Well, maybe we could use in_scope="false" and no value, and let the
 > front end decide...

But the variable may be in scope and the frontend wouldn't be able to
distinguish between this case and when the variable really was out of
scope.  Maybe another value e.g in_scope="unreadable" would work.

Re your patch:

  if (initial && changeable)
    ...
  else if (changeable)
    ...

Isn't:

if (changeable)
  {
    if (initial)
      ...
    else
      ...
  }

easier to read?


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

Re: [PATCH] Catch errors in value_get_print_value

[Daniel Jacobowitz]

On Fri, Feb 09, 2007 at 03:20:49PM +1300, Nick Roberts wrote:
>  > > It's much quieter than a window full of error/warning messages.  If
>  > > there's no value available, the frontend doesn't show one.  That seems
>  > > logical to me but perhaps only because I'm used to it.
>  > 
>  > Well, maybe we could use in_scope="false" and no value, and let the
>  > front end decide...
> 
> But the variable may be in scope and the frontend wouldn't be able to
> distinguish between this case and when the variable really was out of
> scope.  Maybe another value e.g in_scope="unreadable" would work.

Maybe.  I'm not going to make any changes now, and you're happy with
what we've got, so let's leave it alone for the moment - we can come
back to it.

> Isn't:
> 
> if (changeable)
>   {
>     if (initial)
>       ...
>     else
>       ...
>   }
> 
> easier to read?

In my opinion?  No, not really.

-- 
Daniel Jacobowitz
CodeSourcery