Re: Patch for bug2457

[Vladimir Prus <vladimir at codesourcery dot 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);