RFC gdb crashes on watchpoint that's no longer valid

RFC gdb crashes on watchpoint that's no longer valid

[Jason Molenda]

Hi all, 

We came across this recently - if you have a watchpoint on a global
variable, you edit the source to change the name of that var & recompile,
you re-run the inferior in that same gdb session, and gdb dumps core.
Specifically, given a file of

int foo; main () { foo++; foo++; }

You do

% gdb testprog
(gdb) watch foo
(gdb) run
{hit watchpoint}
{edit source of file, change it to}

int fooer; main () { fooer++; fooer++; }

{and recompile}
(gdb) run
{restart inferior?  Yes!  Re-reading symbols..}
{gdb segv}

When gdb re-runs, breakpoint_re_set calls breakpoint_re_set_one
via catch_errors.  In breakpoint_re_set_one we have

      /* So for now, just use a global context.  */
      if (b->exp)
        xfree (b->exp);     
      b->exp = parse_expression (b->exp_string);

If b->exp_string is "foo", and "foo" no longer exists, parse_expression
will unwind up through catch_errors (printing "Error in re-setting
breakpoint %d").  Before that happens, though, the breakpoint's 
expression has been freed so b->exp points to undefined data.

When insert_breakpoints is eventually called, the garbage expression
causes gdb to crash with a stack like this:

#0  evaluate_subexp (expect_type=0x0, exp=0x0, pos=0xbfffd3e4, 
    noside=EVAL_NORMAL) at ../../src/gdb/eval.c:69
#1  0x0807926d in evaluate_expression (exp=0x0) at ../../src/gdb/eval.c:158
#2  0x080fc444 in insert_breakpoints () at ../../src/gdb/breakpoint.c:928

The most obvious fix for this is to goober-up breakpoint_re_set_one a bit.
For instance, here's a change that keeps in the spirit of b_r_s_o and leaves
the breakpoint disabled with its old expression block:

      /* So for now, just use a global context.  */
      save_enable = b->enable_state;
      b->enable_state = bp_disabled;  
      s_exp = parse_expression (b->exp_string);
      if (b->exp)
        xfree (b->exp);
      b->exp = s_exp;
      b->enable_state = save_enable;

The other way we've thought of dealing with this is to delete the
watchpoint altogether, something like this:

      /* If symbols have changed so a saved global watchpoint no longer 
         applies, delete it, lest gdb crash ungloriously. */
      s = b->exp_string;
      if (! gdb_parse_exp_1 (&s, innermost_block, 0, &(b->exp)))
        {
          warning ("Unable to reset watchpoint %d (unable to "
                   "parse expression); deleting", b->number);
          delete_breakpoint (b);
          return 0;
        }
      xfree (s);

Do either of these approaches appeal to anyone?  It's a pretty rare
set of circumstances, but a debugger crash is an unfriendly way of
saying "That expression is longer valid".

Jason

Re: RFC gdb crashes on watchpoint that's no longer valid

[Jason Molenda]

On Tue, Aug 13, 2002 at 03:21:24PM -0700, Jason Molenda wrote:

>       /* If symbols have changed so a saved global watchpoint no longer 
>          applies, delete it, lest gdb crash ungloriously. */
>       s = b->exp_string;
>       if (! gdb_parse_exp_1 (&s, innermost_block, 0, &(b->exp)))
>         {
>           warning ("Unable to reset watchpoint %d (unable to "
>                    "parse expression); deleting", b->number);
>           delete_breakpoint (b);
>           return 0;
>         }
>       xfree (s);


(yes yes, I know.  It's a cut-and-paste-o.  But you get the idea.)

Re: RFC gdb crashes on watchpoint that's no longer valid

[Eli Zaretskii]

> Date: Tue, 13 Aug 2002 15:21:24 -0700
> From: Jason Molenda <jason-swarelist@molenda.com>
> 
> Do either of these approaches appeal to anyone?

My vote is for removing the watchpoint, but I think the message
should be more clear.  For example:

      if (! gdb_parse_exp_1 (&s, innermost_block, 0, &(b->exp)))
        {
          warning ("Unable to reset watchpoint %d (some of "
                   "its variables don't exist); deleting", b->number);

Re: RFC gdb crashes on watchpoint that's no longer valid

[Andrew Cagney]

>> Date: Tue, 13 Aug 2002 15:21:24 -0700
>> From: Jason Molenda <jason-swarelist@molenda.com>
>> 
>> Do either of these approaches appeal to anyone?
> 
> 
> My vote is for removing the watchpoint, but I think the message
> should be more clear.  For example:
> 
>       if (! gdb_parse_exp_1 (&s, innermost_block, 0, &(b->exp)))
>         {
>           warning ("Unable to reset watchpoint %d (some of "
>                    "its variables don't exist); deleting", b->number);

Mine is for: do the same as for breakpoints (unless that is also 
dump-core :-)

Andrew