Re: [ob] unbreak MI

[Michael Snyder <msnyder@specifix.com>]

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