Watchpoint segfaults

Watchpoint segfaults

[Daniel Jacobowitz]

This is just a brain dump; I was going to fix this today, but it was only
tangential to the patch I was working on, and it turned out to be a real
rathole.

There are lots of ways to make gdb segfault with watchpoints.  Here's one of
the easiest to reproduce:

  file ./gdb
  watch object_files
  file
  info break

b->exp will be NULL because re-parsing the expression failed in
breakpoint_re_set_one.  When we go to print the expression we crash.
I think we want to temporarily disable breakpoints whose expression can't be
parsed, and print it as a string rather than using the expression printer.

A frequent cause of this in my debugging sessions is "watch *$33" followed
by something that rereads the symbol table; as Andrew S. noticed, that
clears the history.  Alternative to his patch coming right up which prevents
that particular case, by not clearing the history.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Watchpoint segfaults

[Eli Zaretskii]

> Date: Fri, 9 Dec 2005 14:58:11 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> I think we want to temporarily disable breakpoints whose expression can't be
> parsed

I agree.  But perhaps instead of just disabling it we should display
some warning the first time we find that we are unable to parse its
expression.

> and print it as a string rather than using the expression printer.

Print it where? in the table displayed by "info breakpoints" or
equivalent commands?  Or do you have some other situation in mind?

Re: Watchpoint segfaults

[Daniel Jacobowitz]

On Sat, Dec 10, 2005 at 10:31:02AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 9 Dec 2005 14:58:11 -0500
> > From: Daniel Jacobowitz <drow@false.org>
> > 
> > I think we want to temporarily disable breakpoints whose expression can't be
> > parsed
> 
> I agree.  But perhaps instead of just disabling it we should display
> some warning the first time we find that we are unable to parse its
> expression.

That sounds like a good idea.  We should also try to re-parse it at any
available opportunity, e.g. when the symbol table changes.  This'll
take a little work to get right.

> > and print it as a string rather than using the expression printer.
> 
> Print it where? in the table displayed by "info breakpoints" or
> equivalent commands?  Or do you have some other situation in mind?

Sorry, yes, I was referring to the "info breakpoints" segfault. 
Basically someone's going to have to go through and audit the uses of
->exp.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Watchpoint segfaults

[Eli Zaretskii]

> Date: Sat, 10 Dec 2005 10:57:41 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> > I agree.  But perhaps instead of just disabling it we should display
> > some warning the first time we find that we are unable to parse its
> > expression.
> 
> That sounds like a good idea.  We should also try to re-parse it at any
> available opportunity, e.g. when the symbol table changes.  This'll
> take a little work to get right.

A new flag in the watchpoint expression structure?

> Basically someone's going to have to go through and audit the uses of
> ->exp.

Right.

Re: Watchpoint segfaults

[Ramana Radhakrishnan]

Hi Daniel,


On Sat, 2005-12-10 at 10:57 -0500, Daniel Jacobowitz wrote:
> On Sat, Dec 10, 2005 at 10:31:02AM +0200, Eli Zaretskii wrote:
> > > Date: Fri, 9 Dec 2005 14:58:11 -0500
> > > From: Daniel Jacobowitz <drow@false.org>
> > > 
> > > I think we want to temporarily disable breakpoints whose expression can't be
> > > parsed
> > 
> > I agree.  But perhaps instead of just disabling it we should display
> > some warning the first time we find that we are unable to parse its
> > expression.
> 
> That sounds like a good idea.  We should also try to re-parse it at any
> available opportunity, e.g. when the symbol table changes.  This'll
> take a little work to get right.

I am not too clear about this. Do you mean
observer_notify_executable_changed  but that does not get called in case
of add-symbol-file where the symtab could change again and you have
potential cases there ? There used to be a file_changed_hook where we
might be able to do this but thats been deprecated here . 


http://sources.redhat.com/ml/gdb-patches/2004-04/msg00271.html

> 
> > > and print it as a string rather than using the expression printer.
> > 
> > Print it where? in the table displayed by "info breakpoints" or
> > equivalent commands?  Or do you have some other situation in mind?
> 
> Sorry, yes, I was referring to the "info breakpoints" segfault. 
> Basically someone's going to have to go through and audit the uses of
> ->exp.

This is a rather simplistic fix which does not yet disable the
watchpoint but prevents the segfault .  I did on the 6.4 branch since
that is what I have handy .  Also the other occurences of
print_expression with -->exp that I could find were in mention in which
case you do have a symbol table already created and if the symbol were
not there an automatic error would be raised much in advance. 

Is there any other usecase you had in mind ? 


cheers
Ramana

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.217
diff -a -u -r1.217 breakpoint.c
--- breakpoint.c        29 May 2005 03:13:17 -0000      1.217
+++ breakpoint.c        10 Dec 2005 19:38:54 -0000
@@ -3387,7 +3387,10 @@
        if (addressprint)
          ui_out_field_skip (uiout, "addr");
        annotate_field (5);
-       print_expression (b->exp, stb->stream);
+       if (b->exp)
+         print_expression (b->exp, stb->stream);
+       else
+         ui_out_field_string (uiout,"what", b->exp_string);
        ui_out_field_stream (uiout, "what", stb);
        break;