RFC gdb crashes on watchpoint that's no longer valid

[Jason Molenda <jason-swarelist at molenda dot com>]

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