Re: [ob] unbreak MI

[Vladimir Prus <ghost@cs.msu.su>]

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