Patch for bug2457

Patch for bug2457

[Francois Chouinard]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdb-bug2457.patch --]
[-- Type: text/x-diff; name=gdb-bug2457.patch, Size: 1334 bytes --]

### 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 */
+	  }
 
 	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));
 	  }
 	breakpoints_changed ();
 	breakpoint_modify_event (b->number);

Re: Patch for bug2457

[Vladimir Prus]

> 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);