Re: Patch for bug2457

[Vladimir Prus <vladimir@codesourcery.com>]

> Hi,
> 
> Here's a simple patch for bug2457 (Incorrect handling of erroneous
> breakpoint conditions).
> 
> It just postpones the update of the breakpoint condition until it has
> been parsed successfully.
> 
> If there is a parsing error, the condition is set to NULL.
> 
> Best Regards,
> /fc
> gdb-bug2457.patch
>   ### Eclipse Workspace Patch 1.0 
> #P gdb 
> Index: breakpoint.c 
> =================================================================== 
> RCS file: /cvs/src/src/gdb/breakpoint.c,v 
> retrieving revision 1.321 
> diff -u -r1.321 breakpoint.c 
> --- breakpoint.c        4 May 2008 19:38:59 -0000       1.321 
> +++ breakpoint.c        14 May 2008 17:32:06 -0000 
> @@ -596,11 +596,13 @@ 
>               } 
>           } 
>         if (b->cond_string != NULL) 
> -         xfree (b->cond_string); 
> +         { 
> +           xfree (b->cond_string); 
> +           b->cond_string = NULL;      /* Keep in sync */ 
> +         } 

Is this actually what we want? If we want the breakpoint condition to be
unchanged on error, then we should not clear b->cond_string here.

>   
>         if (*p == 0) 
>           { 
> -           b->cond_string = NULL; 
>             if (from_tty) 
>               printf_filtered (_("Breakpoint %d now unconditional.\n"), bnum); 
>           } 
> @@ -609,7 +611,6 @@ 
>             arg = p; 
>             /* I don't know if it matters whether this is the string the user 
>                typed in or the decompiled expression.  */ 
> -           b->cond_string = savestring (arg, strlen (arg)); 
>             b->condition_not_parsed = 0; 
>             for (loc = b->loc; loc; loc = loc->next) 
>               { 
> @@ -619,6 +620,9 @@ 
>                 if (*arg) 
>                   error (_("Junk at end of expression")); 
>               } 
> +           /* If we get here, the condition was parsed successfully and 
> +           no exception was thrown. See bug 2457. */ 
> +           b->cond_string = savestring (p, strlen (p)); 

Presumably, if an exception is thrown, then we should undo the changes to breakpoint
location's 'cond' fields that we did above? Alternatively, we can:

1. Re-evaluate new condition string at all locations, and store the results in
a vector
2. If successfull, store new expressions in loc->cond, foreach loc.
3. Update b->cond_string

That would be actually "strong exception safety" in C++ terms.

There are some technical issues with the patch:
1. There should be a changelog entry
2. Comments should be meaningful on their own, so I think "See bug 2457" is not
good -- there should be explanation that we want exception-safety.
3. Comment like "Keep in sync" also does not say anything.

Thanks,
Volodya


>           } 
>         breakpoints_changed (); 
>         breakpoint_modify_event (b->number);