[ob] unbreak MI

[ob] unbreak MI

[Vladimir Prus]

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


It appears that some of recent cleanups broke MI, because
check_typedef is getting passed a NULL pointer -- this
suggest someone did not run a testsuite prior to commit ;-).

It does not matter which particular patch broke it, because
it's trivially fixable by the attached patch, checked in
as obvious.

- Volodya


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

? 1.diff
? 2.diff
? 3.diff
? unbreak_mi.diff
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.8659
diff -u -p -r1.8659 ChangeLog
--- ChangeLog	30 Aug 2007 13:13:58 -0000	1.8659
+++ ChangeLog	31 Aug 2007 18:41:20 -0000
@@ -1,3 +1,8 @@
+2007-08-31  Vladimir Prus  <vladimir@codesourcery.com>
+
+	* mi/mi-cmd-var.c (print_varobj): If a varobj
+	type is NULL, don't try to print it.
+	
 2007-08-30  Alan Modra  <amodra@bigpond.net.au>
 
 	* ppc-linux-nat.c (right_fill_reg): Delete.
Index: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.38
diff -u -p -r1.38 mi-cmd-var.c
--- mi/mi-cmd-var.c	28 Aug 2007 20:34:18 -0000	1.38
+++ mi/mi-cmd-var.c	31 Aug 2007 18:41:20 -0000
@@ -47,6 +47,7 @@ static void 
 print_varobj (struct varobj *var, enum print_values print_values,
 	      int print_expression)
 {
+  struct type *gdb_type;
   char *type;
 
   ui_out_field_string (uiout, "name", varobj_get_objname (var));
@@ -54,7 +55,8 @@ print_varobj (struct varobj *var, enum p
     ui_out_field_string (uiout, "exp", varobj_get_expression (var));
   ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
   
-  if (mi_print_value_p (varobj_get_gdb_type (var), print_values))
+  gdb_type = varobj_get_gdb_type (var);
+  if (gdb_type && mi_print_value_p (gdb_type, print_values))
     ui_out_field_string (uiout, "value", varobj_get_value (var));
 
   type = varobj_get_type (var);

Re: [ob] unbreak MI

[Nick Roberts]

 > It appears that some of recent cleanups broke MI, because
 > check_typedef is getting passed a NULL pointer -- this
 > suggest someone did not run a testsuite prior to commit ;-).
 > 
 > It does not matter which particular patch broke it, because
 > it's trivially fixable by the attached patch, checked in
 > as obvious.

This fix (now in GDB 6.7) breaks watch expressions of C++ objects on Emacs
because the MI command "-var-list-children --all-values" no longer always
includes the value field:

-  if (mi_print_value_p (varobj_get_gdb_type (var), print_values))
+  gdb_type = varobj_get_gdb_type (var);
+  if (gdb_type && mi_print_value_p (gdb_type, print_values))
     ui_out_field_string (uiout, "value", varobj_get_value (var));


Generally, with a NULL pointer, or and address that can't be dereferenced,
MI prints out the value field as value="".

What is the problem in this case?  Why isn't the right fix to add a
check_typedef somewhere?

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

Re: [ob] unbreak MI

[Vladimir Prus]

Nick Roberts wrote:

>  > It appears that some of recent cleanups broke MI, because
>  > check_typedef is getting passed a NULL pointer -- this
>  > suggest someone did not run a testsuite prior to commit ;-).
>  > 
>  > It does not matter which particular patch broke it, because
>  > it's trivially fixable by the attached patch, checked in
>  > as obvious.
> 
> This fix (now in GDB 6.7) breaks watch expressions of C++ objects on Emacs
> because the MI command "-var-list-children --all-values" no longer always
> includes the value field:
> 
> -  if (mi_print_value_p (varobj_get_gdb_type (var), print_values))
> +  gdb_type = varobj_get_gdb_type (var);
> +  if (gdb_type && mi_print_value_p (gdb_type, print_values))
>      ui_out_field_string (uiout, "value", varobj_get_value (var));
> 
> 
> Generally, with a NULL pointer, or and address that can't be dereferenced,
> MI prints out the value field as value="".
> 
> What is the problem in this case?  Why isn't the right fix to add a
> check_typedef somewhere?

check_typedef? The original problem was that check_typedef was getting
called on NULL pointer, so adding more check_typedef calls won't help.
Probably:

        if (!gdb_type)
                ui_out_field_string (uiout, "value", "");
        else if (mi_print_value_p (gdb_type, print_values))
                ui_out_field_string (uiout, "value", varobj_get_value (var));

is the right logic?

- Volodya

        

Re: [ob] unbreak MI

[Nick Roberts]

 > > Generally, with a NULL pointer, or and address that can't be dereferenced,
 > > MI prints out the value field as value="".
 > > 
 > > What is the problem in this case?  Why isn't the right fix to add a
 > > check_typedef somewhere?
 > 
 > check_typedef? The original problem was that check_typedef was getting
 > called on NULL pointer, so adding more check_typedef calls won't help.
 > Probably:
 > 
 >         if (!gdb_type)
 >                 ui_out_field_string (uiout, "value", "");
 >         else if (mi_print_value_p (gdb_type, print_values))
 >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
 > 
 > is the right logic?

It's probably the right logic, but it seems to cure the symptom rather than the
cause.  What I mean't, I guess, was where/how does check_typedef is get passed
a NULL pointer?  And can't that call be conditioned (i.e. "add a *check* to
check_typedef") , e.g., something like:

if (!gdb_type)
   check_typedef (gdb_type)

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

Re: [ob] unbreak MI

[Vladimir Prus]

On Tuesday 27 November 2007 09:27:37 Nick Roberts wrote:
>  > > Generally, with a NULL pointer, or and address that can't be dereferenced,
>  > > MI prints out the value field as value="".
>  > > 
>  > > What is the problem in this case?  Why isn't the right fix to add a
>  > > check_typedef somewhere?
>  > 
>  > check_typedef? The original problem was that check_typedef was getting
>  > called on NULL pointer, so adding more check_typedef calls won't help.
>  > Probably:
>  > 
>  >         if (!gdb_type)
>  >                 ui_out_field_string (uiout, "value", "");
>  >         else if (mi_print_value_p (gdb_type, print_values))
>  >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
>  > 
>  > is the right logic?
> 
> It's probably the right logic, but it seems to cure the symptom rather than the
> cause.  What I mean't, I guess, was where/how does check_typedef is get passed
> a NULL pointer?  And can't that call be conditioned (i.e. "add a *check* to
> check_typedef") , e.g., something like:
> 
> if (!gdb_type)
>    check_typedef (gdb_type)

Just look at mi_print_value_p, and you'll see a call to check_typedef. Actually,
the code previously looked like:

	if (type != NULL)
		type = check_typedef (type);

It was changed in revision 1.38, with the following comment:

	2007-08-28  Michael Snyder  <msnyder@access-company.com>

	* mi/mi-cmd-var.c (mi_print_value_p): No longer necessary to
	check for null before calling check_typedef.

However, apparently check_typedef still crashes when passed NULL,
and it can be passed NULL.

The original code, in fact, was in error too, because of this:

  return (TYPE_CODE (type) != TYPE_CODE_ARRAY
	  && TYPE_CODE (type) != TYPE_CODE_STRUCT
	  && TYPE_CODE (type) != TYPE_CODE_UNION);	

This will crash if 'type' is NULL. Testsuite fails to detect this because presently
type is NULL only for C++ pseudo-fields  ('public'/'private') and the code
above is only executed for --simple-values.

Does this clarify things?

- Volodya

Re: [ob] unbreak MI

[Nick Roberts]

 > >  > Probably:
 > >  > 
 > >  >         if (!gdb_type)
 > >  >                 ui_out_field_string (uiout, "value", "");
 > >  >         else if (mi_print_value_p (gdb_type, print_values))
 > >  >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
 > >  > 
 > >  > is the right logic?
 > > 
 > > It's probably the right logic, but it seems to cure the symptom rather
 > > than the cause.

Looking again, I see it's not the right logic. GDB will print a value=""
field even for "--no-values" when gdb_type is NULL.

 >...
 > The original code, in fact, was in error too, because of this:
 > 
 >   return (TYPE_CODE (type) != TYPE_CODE_ARRAY
 > 	  && TYPE_CODE (type) != TYPE_CODE_STRUCT
 > 	  && TYPE_CODE (type) != TYPE_CODE_UNION);	
 > 
 > This will crash if 'type' is NULL. Testsuite fails to detect this because
 > presently type is NULL only for C++ pseudo-fields ('public'/'private') and
 > the code above is only executed for --simple-values.

I never use, and never wanted, "-var-list-children --simple-values".

 > Does this clarify things?

Yes, thanks.  I think the patch below should cover all cases.

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



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

	* mi/mi-cmd-var.c (print_varobj): Revert change on 2007-08-31.
	(mi_print_value_p): Guard against type = NULL.



*** mi-cmd-var.c.~1.41.~	2007-11-21 08:19:11.000000000 +1300
--- mi-cmd-var.c	2007-11-27 23:23:38.000000000 +1300
*************** print_varobj (struct varobj *var, enum p
*** 55,62 ****
      ui_out_field_string (uiout, "exp", varobj_get_expression (var));
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    
!   gdb_type = varobj_get_gdb_type (var);
!   if (gdb_type && mi_print_value_p (gdb_type, print_values))
      ui_out_field_string (uiout, "value", varobj_get_value (var));
  
    type = varobj_get_type (var);
--- 55,61 ----
      ui_out_field_string (uiout, "exp", varobj_get_expression (var));
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    
!   if (mi_print_value_p (varobj_get_gdb_type (var), print_values))
      ui_out_field_string (uiout, "value", varobj_get_value (var));
  
    type = varobj_get_type (var);
*************** Must be: 0 or \"%s\", 1 or \"%s\", 2 or 
*** 327,333 ****
  static int
  mi_print_value_p (struct type *type, enum print_values print_values)
  {
-   type = check_typedef (type);
  
    if (print_values == PRINT_NO_VALUES)
      return 0;
--- 326,331 ----
*************** mi_print_value_p (struct type *type, enu
*** 335,346 ****
    if (print_values == PRINT_ALL_VALUES)
      return 1;
  
!   /* For PRINT_SIMPLE_VALUES, only print the value if it has a type
!      and that type is not a compound type.  */
  
!   return (TYPE_CODE (type) != TYPE_CODE_ARRAY
! 	  && TYPE_CODE (type) != TYPE_CODE_STRUCT
! 	  && TYPE_CODE (type) != TYPE_CODE_UNION);
  }
  
  enum mi_cmd_result
--- 333,350 ----
    if (print_values == PRINT_ALL_VALUES)
      return 1;
  
!   if (type == NULL)
!     return 1;
!   else
!     {
!       type = check_typedef (type);
  
!       /* For PRINT_SIMPLE_VALUES, only print the value if it has a type
! 	 and that type is not a compound type.  */
!       return (TYPE_CODE (type) != TYPE_CODE_ARRAY
! 	      && TYPE_CODE (type) != TYPE_CODE_STRUCT
! 	      && TYPE_CODE (type) != TYPE_CODE_UNION);
!     }
  }
  
  enum mi_cmd_result

Re: [ob] unbreak MI

[Vladimir Prus]

On Tuesday 27 November 2007 13:32:54 Nick Roberts wrote:
>  > >  > Probably:
>  > >  > 
>  > >  >         if (!gdb_type)
>  > >  >                 ui_out_field_string (uiout, "value", "");
>  > >  >         else if (mi_print_value_p (gdb_type, print_values))
>  > >  >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
>  > >  > 
>  > >  > is the right logic?
>  > > 
>  > > It's probably the right logic, but it seems to cure the symptom rather
>  > > than the cause.
> 
> Looking again, I see it's not the right logic. GDB will print a value=""
> field even for "--no-values" when gdb_type is NULL.
> 
>  >...
>  > The original code, in fact, was in error too, because of this:
>  > 
>  >   return (TYPE_CODE (type) != TYPE_CODE_ARRAY
>  > 	  && TYPE_CODE (type) != TYPE_CODE_STRUCT
>  > 	  && TYPE_CODE (type) != TYPE_CODE_UNION);	
>  > 
>  > This will crash if 'type' is NULL. Testsuite fails to detect this because
>  > presently type is NULL only for C++ pseudo-fields ('public'/'private') and
>  > the code above is only executed for --simple-values.
> 
> I never use, and never wanted, "-var-list-children --simple-values".

And? You keep on making this argument in various context, and I find it wrong. It does
not matter if you, or I, make use of a feature -- the code that can segfault on a
usage documented in gdb manual should either be removed, or fixed.

>  > Does this clarify things?
> 
> Yes, thanks.  I think the patch below should cover all cases.

As far as I'm concerned, it's fine.

- Volodya

Re: [ob] unbreak MI

[Nick Roberts]

 > > I never use, and never wanted, "-var-list-children --simple-values".
 > 
 > And? You keep on making this argument in various context, and I find it
 > wrong. It does not matter if you, or I, make use of a feature -- the code
 > that can segfault on a usage documented in gdb manual should either be
 > removed, or fixed.

Its not an argument but a statement, so it can't really be wrong in your sense.
I'm not sure I've made it that often but what it means is that I haven't tested
this option with this command.

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

Re: [ob] unbreak MI

[Michael Snyder]

On Tue, 2007-11-27 at 10:00 +0300, Vladimir Prus wrote:
> On Tuesday 27 November 2007 09:27:37 Nick Roberts wrote:
> >  > > Generally, with a NULL pointer, or and address that can't be dereferenced,
> >  > > MI prints out the value field as value="".
> >  > > 
> >  > > What is the problem in this case?  Why isn't the right fix to add a
> >  > > check_typedef somewhere?
> >  > 
> >  > check_typedef? The original problem was that check_typedef was getting
> >  > called on NULL pointer, so adding more check_typedef calls won't help.
> >  > Probably:
> >  > 
> >  >         if (!gdb_type)
> >  >                 ui_out_field_string (uiout, "value", "");
> >  >         else if (mi_print_value_p (gdb_type, print_values))
> >  >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
> >  > 
> >  > is the right logic?
> > 
> > It's probably the right logic, but it seems to cure the symptom rather than the
> > cause.  What I mean't, I guess, was where/how does check_typedef is get passed
> > a NULL pointer?  And can't that call be conditioned (i.e. "add a *check* to
> > check_typedef") , e.g., something like:
> > 
> > if (!gdb_type)
> >    check_typedef (gdb_type)
> 
> Just look at mi_print_value_p, and you'll see a call to check_typedef. Actually,
> the code previously looked like:
> 
> 	if (type != NULL)
> 		type = check_typedef (type);
> 
> It was changed in revision 1.38, with the following comment:
> 
> 	2007-08-28  Michael Snyder  <msnyder@access-company.com>
> 
> 	* mi/mi-cmd-var.c (mi_print_value_p): No longer necessary to
> 	check for null before calling check_typedef.
> 
> However, apparently check_typedef still crashes when passed NULL,
> and it can be passed NULL.

It doesn't crash -- it calls assert, therefore abort.

The debate at the time was whether it made more sense
to check for null before every call to check_typedef, 
or simply to have check_typedef do the check for null
itself.

Makeing a change in one place seemed easier than makeing
a change in 100's of places.

And it's not clear that check_typedef can do anything
intelligent to recover if a null pointer is passed ---
hence the abort.

Probably calling error rather than abort would be acceptable.


> 
> The original code, in fact, was in error too, because of this:
> 
>   return (TYPE_CODE (type) != TYPE_CODE_ARRAY
> 	  && TYPE_CODE (type) != TYPE_CODE_STRUCT
> 	  && TYPE_CODE (type) != TYPE_CODE_UNION);	
> 
> This will crash if 'type' is NULL. Testsuite fails to detect this because presently
> type is NULL only for C++ pseudo-fields  ('public'/'private') and the code
> above is only executed for --simple-values.
> 
> Does this clarify things?
> 
> - Volodya

Re: [ob] unbreak MI

[Vladimir Prus]

On Tuesday 27 November 2007 23:34:58 Michael Snyder wrote:
> On Tue, 2007-11-27 at 10:00 +0300, Vladimir Prus wrote:
> > On Tuesday 27 November 2007 09:27:37 Nick Roberts wrote:
> > >  > > Generally, with a NULL pointer, or and address that can't be dereferenced,
> > >  > > MI prints out the value field as value="".
> > >  > > 
> > >  > > What is the problem in this case?  Why isn't the right fix to add a
> > >  > > check_typedef somewhere?
> > >  > 
> > >  > check_typedef? The original problem was that check_typedef was getting
> > >  > called on NULL pointer, so adding more check_typedef calls won't help.
> > >  > Probably:
> > >  > 
> > >  >         if (!gdb_type)
> > >  >                 ui_out_field_string (uiout, "value", "");
> > >  >         else if (mi_print_value_p (gdb_type, print_values))
> > >  >                 ui_out_field_string (uiout, "value", varobj_get_value (var));
> > >  > 
> > >  > is the right logic?
> > > 
> > > It's probably the right logic, but it seems to cure the symptom rather than the
> > > cause.  What I mean't, I guess, was where/how does check_typedef is get passed
> > > a NULL pointer?  And can't that call be conditioned (i.e. "add a *check* to
> > > check_typedef") , e.g., something like:
> > > 
> > > if (!gdb_type)
> > >    check_typedef (gdb_type)
> > 
> > Just look at mi_print_value_p, and you'll see a call to check_typedef. Actually,
> > the code previously looked like:
> > 
> > 	if (type != NULL)
> > 		type = check_typedef (type);
> > 
> > It was changed in revision 1.38, with the following comment:
> > 
> > 	2007-08-28  Michael Snyder  <msnyder@access-company.com>
> > 
> > 	* mi/mi-cmd-var.c (mi_print_value_p): No longer necessary to
> > 	check for null before calling check_typedef.
> > 
> > However, apparently check_typedef still crashes when passed NULL,
> > and it can be passed NULL.
> 
> It doesn't crash -- it calls assert, therefore abort.

Well, further debugging is no longer possible, in any case.

> The debate at the time was whether it made more sense
> to check for null before every call to check_typedef, 
> or simply to have check_typedef do the check for null
> itself.
> 
> Makeing a change in one place seemed easier than makeing
> a change in 100's of places.

Well, I think the gdb_assert in check_typedef is a right
thing...

 
> And it's not clear that check_typedef can do anything
> intelligent to recover if a null pointer is passed ---
> hence the abort.

... since indeed, check_typedef can't do anything with
a null pointer.

Ideally, all those 100 places should be examined to
understand if NULL can be passed now, and what's
the right behaviour should be. But as the only
problem was with MI, we probably need not look
at remaining 99 places right now.

- Volodya

Re: [ob] unbreak MI

[Nick Roberts]

 > Probably calling error rather than abort would be acceptable.

Please don't.  It was the change to print_varobj that caused a problem for
me, not the one to check_typedef.

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

Re: [ob] unbreak MI

[Daniel Jacobowitz]

On Tue, Nov 27, 2007 at 04:07:41PM +1300, Nick Roberts wrote:
> Generally, with a NULL pointer, or and address that can't be dereferenced,
> MI prints out the value field as value="".

If this is a requirement, could it be added to the manual?  I'd have
naively expected no value to not be printed.

Your patch is OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [ob] unbreak MI

[Nick Roberts]

 > > Generally, with a NULL pointer, or and address that can't be dereferenced,
 > > MI prints out the value field as value="".
 > 
 > If this is a requirement, could it be added to the manual?  I'd have
 > naively expected no value to not be printed.

That's exactly what does happen from the perspective of the front end: it
takes the null string and prints it, i.e., nothing.

I'm not sure what would be worthwhile to say and I never considered this
situation when writing the front end for Emacs; it just came out right.
Perhaps, however, I've just got used to the status quo, and you could ask
others (Ericsson?) if they find this behaviour surprising.

 > Your patch is OK.

Committed.  Thanks.

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