[5/9] Associate parsed condition with location

[5/9] Associate parsed condition with location

[Vladimir Prus]

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


This patch moves the 'cond' field from breakpoint
to bp_location.  The 'cond' field is parsed expression,
and for multiple locations, the parsed expression is different
depending on the context where it's parsed. So we need to
have 'cond' in bp_location. OK?

- Volodya


[-- Attachment #2: 5.ChangeLog --]
[-- Type: text/plain, Size: 156 bytes --]

	gdb/
	* breakpoint.h (struct breakpoint): Move the cond 
	field to...
	(struct bp_location): Here.
	* breakpoint.c: Adjust.
	* tui/tui-winsource.c: Adjust.

[-- Attachment #3: mainline_5_per_loc_cond.diff --]
[-- Type: text/x-diff, Size: 9449 bytes --]

--- gdb/breakpoint.c	(/work/mb_mainline/4_bpstat_owner)	(revision 4741)
+++ gdb/breakpoint.c	(/work/mb_mainline/5_per_loc_cond)	(revision 4741)
@@ -569,17 +569,17 @@ condition_command (char *arg, int from_t
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
     {
-      if (b->cond)
+      struct bp_location *loc = b->loc;
+      if (loc->cond)
 	{
-	  xfree (b->cond);
-	  b->cond = 0;
+	  xfree (loc->cond);
+	  loc->cond = 0;
 	}
       if (b->cond_string != NULL)
 	xfree (b->cond_string);
 
       if (*p == 0)
 	{
-	  b->cond = 0;
 	  b->cond_string = NULL;
 	  if (from_tty)
 	    printf_filtered (_("Breakpoint %d now unconditional.\n"), bnum);
@@ -592,7 +592,8 @@ condition_command (char *arg, int from_t
 	  b->cond_string = savestring (arg, strlen (arg));
 	  if (!b->pending)
 	    {
-	      b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+	      b->loc->cond = parse_exp_1 (&arg, 
+					  block_for_pc (b->loc->address), 0);
 	      if (*arg)
 		error (_("Junk at end of expression"));
 	    }
@@ -2923,19 +2924,19 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
       {
 	int value_is_zero = 0;
 
-	if (b->cond)
+	if (b->loc->cond)
 	  {
 	    /* Need to select the frame, with all that implies
 	       so that the conditions will have the right context.  */
 	    select_frame (get_current_frame ());
 	    value_is_zero
-	      = catch_errors (breakpoint_cond_eval, (b->cond),
+	      = catch_errors (breakpoint_cond_eval, (b->loc->cond),
 			      "Error in testing breakpoint condition:\n",
 			      RETURN_MASK_ALL);
 	    /* FIXME-someday, should give breakpoint # */
 	    free_all_values ();
 	  }
-	if (b->cond && value_is_zero)
+	if (b->loc->cond && value_is_zero)
 	  {
 	    bs->stop = 0;
 	    /* Don't consider this a hit.  */
@@ -3606,14 +3607,14 @@ print_one_breakpoint (struct breakpoint 
       ui_out_text (uiout, "\n");
     }
   
-  if (b->cond && !ada_exception_catchpoint_p (b))
+  if (b->loc->cond && !ada_exception_catchpoint_p (b))
     {
       /* We do not print the condition for Ada exception catchpoints
          because the condition is an internal implementation detail
          that we do not want to expose to the user.  */
       annotate_field (7);
       ui_out_text (uiout, "\tstop only if ");
-      print_expression (b->cond, stb->stream);
+      print_expression (b->loc->cond, stb->stream);
       ui_out_field_stream (uiout, "cond", stb);
       ui_out_text (uiout, "\n");
     }
@@ -4085,6 +4086,7 @@ allocate_bp_location (struct breakpoint 
   memset (loc, 0, sizeof (*loc));
 
   loc->owner = bpt;
+  loc->cond = NULL;
 
   switch (bp_type)
     {
@@ -4604,7 +4606,6 @@ solib_load_unload_1 (char *hookname, int
   b = set_raw_breakpoint (sals.sals[0], bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -4661,7 +4662,6 @@ create_fork_vfork_event_catchpoint (int 
   b = set_raw_breakpoint (sal, bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -4700,7 +4700,6 @@ create_exec_event_catchpoint (int tempfl
   b = set_raw_breakpoint (sal, bp_catch_exec);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ?
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -5033,7 +5032,7 @@ create_breakpoints (struct symtabs_and_l
 	b = set_raw_breakpoint (sal, type);
 	set_breakpoint_count (breakpoint_count + 1);
 	b->number = breakpoint_count;
-	b->cond = cond[i];
+	b->loc->cond = cond[i];
 	b->thread = thread;
 	if (addr_string[i])
 	  b->addr_string = addr_string[i];
@@ -5056,7 +5055,7 @@ create_breakpoints (struct symtabs_and_l
 	      {
 		arg = pending_bp->cond_string;
 		b->cond_string = savestring (arg, strlen (arg));
-		b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+		b->loc->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
 		if (*arg)
 		  error (_("Junk at end of pending breakpoint condition expression"));
 	      }
@@ -5399,7 +5398,7 @@ break_command_1 (char *arg, int flag, in
 		              : bp_breakpoint);
       set_breakpoint_count (breakpoint_count + 1);
       b->number = breakpoint_count;
-      b->cond = *cond;
+      b->loc->cond = *cond;
       b->thread = thread;
       b->addr_string = *addr_string;
       b->cond_string = *cond_string;
@@ -5792,7 +5791,7 @@ watch_command_1 (char *arg, int accessfl
   b->exp_valid_block = exp_valid_block;
   b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
-  b->cond = cond;
+  b->loc->cond = cond;
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -6352,7 +6351,6 @@ create_exception_catchpoint (int tempfla
   b = set_raw_breakpoint (*sal, bptype);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -6432,7 +6430,6 @@ handle_gnu_v3_exceptions (int tempflag, 
   b = set_raw_breakpoint (sals.sals[0], bp_breakpoint);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = -1;
@@ -6519,7 +6516,7 @@ create_ada_exception_breakpoint (struct 
   b->disposition = tempflag ? disp_del : disp_donttouch;
   b->number = breakpoint_count;
   b->ignore_count = 0;
-  b->cond = cond;
+  b->loc->cond = cond;
   b->addr_string = addr_string;
   b->language = language_ada;
   b->cond_string = cond_string;
@@ -6701,7 +6698,6 @@ set_breakpoint_sal (struct symtab_and_li
   b = set_raw_breakpoint (sal, bp_breakpoint);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = 0;
   b->thread = -1;
   return b;
 }
@@ -7021,8 +7017,6 @@ delete_breakpoint (struct breakpoint *bp
     }
 
   free_command_lines (&bpt->commands);
-  if (bpt->cond)
-    xfree (bpt->cond);
   if (bpt->cond_string != NULL)
     xfree (bpt->cond_string);
   if (bpt->addr_string != NULL)
@@ -7062,6 +7056,8 @@ delete_breakpoint (struct breakpoint *bp
      bp, we mark it as deleted before freeing its storage. */
   bpt->type = bp_none;
 
+  if (bpt->loc->cond)
+    xfree (bpt->loc->cond);
   xfree (bpt->loc);
   xfree (bpt);
 }
@@ -7215,14 +7211,14 @@ breakpoint_re_set_one (void *bint)
 	  if (b->cond_string != NULL)
 	    {
 	      s = b->cond_string;
-	      if (b->cond)
+	      if (b->loc->cond)
 		{
-		  xfree (b->cond);
+		  xfree (b->loc->cond);
 		  /* Avoid re-freeing b->exp if an error during the call
 		     to parse_exp_1.  */
-		  b->cond = NULL;
+		  b->loc->cond = NULL;
 		}
-	      b->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 0);
+	      b->loc->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 0);
 	    }
 
 	  /* We need to re-set the breakpoint if the address changes... */
@@ -7314,14 +7310,14 @@ breakpoint_re_set_one (void *bint)
       if (b->cond_string != NULL)
 	{
 	  s = b->cond_string;
-	  if (b->cond)
+	  if (b->loc->cond)
 	    {
-	      xfree (b->cond);
+	      xfree (b->loc->cond);
 	      /* Avoid re-freeing b->exp if an error during the call
 		 to parse_exp_1.  */
-	      b->cond = NULL;
+	      b->loc->cond = NULL;
 	    }
-	  b->cond = parse_exp_1 (&s, (struct block *) 0, 0);
+	  b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0);
 	}
       if (breakpoint_enabled (b))
 	mention (b);
--- gdb/breakpoint.h	(/work/mb_mainline/4_bpstat_owner)	(revision 4741)
+++ gdb/breakpoint.h	(/work/mb_mainline/5_per_loc_cond)	(revision 4741)
@@ -243,6 +243,13 @@ struct bp_location
      than reference counting.  */
   struct breakpoint *owner;
 
+  /* Conditional.  Break only if this expression's value is nonzero.  
+     Unlike string form of condition, which is associated with breakpoint,
+     this is associated with location, since if breakpoint has several
+     PC locations, the evaluation of expression can be different for
+     different PCs.  */
+  struct expression *cond;
+  
   /* Nonzero if this breakpoint is now inserted.  */
   char inserted;
 
@@ -341,8 +348,6 @@ struct breakpoint
     /* Stack depth (address of frame).  If nonzero, break only if fp
        equals this.  */
     struct frame_id frame_id;
-    /* Conditional.  Break only if this expression's value is nonzero.  */
-    struct expression *cond;
 
     /* String we used to set the breakpoint (malloc'd).  */
     char *addr_string;
--- gdb/tui/tui-winsource.c	(/work/mb_mainline/4_bpstat_owner)	(revision 4741)
+++ gdb/tui/tui-winsource.c	(/work/mb_mainline/5_per_loc_cond)	(revision 4741)
@@ -449,7 +449,7 @@ tui_update_breakpoint_info (struct tui_w
                 mode |= TUI_BP_ENABLED;
               if (bp->hit_count)
                 mode |= TUI_BP_HIT;
-              if (bp->cond)
+              if (bp->loc->cond)
                 mode |= TUI_BP_CONDITIONAL;
               if (bp->type == bp_hardware_breakpoint)
                 mode |= TUI_BP_HARDWARE;

Re: [5/9] Associate parsed condition with location

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 8 Sep 2007 00:42:24 +0400
> 
> 	* breakpoint.c: Adjust.
> 	* tui/tui-winsource.c: Adjust.

Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
because they don't name the functions where the changes were made.

> --- gdb/breakpoint.h	(/work/mb_mainline/4_bpstat_owner)	(revision 4741)
> +++ gdb/breakpoint.h	(/work/mb_mainline/5_per_loc_cond)	(revision 4741)
> @@ -243,6 +243,13 @@ struct bp_location
>       than reference counting.  */
>    struct breakpoint *owner;
>  
> +  /* Conditional.  Break only if this expression's value is nonzero.  
> +     Unlike string form of condition, which is associated with breakpoint,
> +     this is associated with location, since if breakpoint has several
> +     PC locations, the evaluation of expression can be different for
> +     different PCs.  */
> +  struct expression *cond;

I think we should use ``location'' instead of ``PC'', since you make
the change for watchpoints as well, where the PC is irrelevant.

Re: [5/9] Associate parsed condition with location

[Vladimir Prus]

Eli Zaretskii wrote:

>> From: Vladimir Prus <vladimir@codesourcery.com>
>> Date: Sat, 8 Sep 2007 00:42:24 +0400
>> 
>> * breakpoint.c: Adjust.
>> * tui/tui-winsource.c: Adjust.
> 
> Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
> because they don't name the functions where the changes were made.

Yes; however this is highly mechanical change, there's nothing specific
I can say about any function.

>> --- gdb/breakpoint.h (/work/mb_mainline/4_bpstat_owner)      (revision 4741)
>> +++ gdb/breakpoint.h (/work/mb_mainline/5_per_loc_cond)      (revision 4741)
>> @@ -243,6 +243,13 @@ struct bp_location
>>       than reference counting.  */
>>    struct breakpoint *owner;
>>  
>> +  /* Conditional.  Break only if this expression's value is nonzero.
>> +     Unlike string form of condition, which is associated with
>> breakpoint,
>> +     this is associated with location, since if breakpoint has several
>> +     PC locations, the evaluation of expression can be different for
>> +     different PCs.  */
>> +  struct expression *cond;
> 
> I think we should use ``location'' instead of ``PC'', since you make
> the change for watchpoints as well, where the PC is irrelevant.

Point taken, I'll see if I can reword this.

- Volodya

Re: [5/9] Associate parsed condition with location

[Eli Zaretskii]

> From:  Vladimir Prus <ghost@cs.msu.su>
> Date:  Sat, 08 Sep 2007 15:27:11 +0400
> 
> Eli Zaretskii wrote:
> 
> >> From: Vladimir Prus <vladimir@codesourcery.com>
> >> Date: Sat, 8 Sep 2007 00:42:24 +0400
> >> 
> >> * breakpoint.c: Adjust.
> >> * tui/tui-winsource.c: Adjust.
> > 
> > Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
> > because they don't name the functions where the changes were made.
> 
> Yes; however this is highly mechanical change, there's nothing specific
> I can say about any function.

We only need the function names, that's all.  A year from now, someone
who will want to find out when this change was done in one of those
functions, will grep the ChangeLog's for the function's name.

Re: [5/9] Associate parsed condition with location

[Daniel Jacobowitz]

On Sat, Sep 08, 2007 at 03:17:06PM +0300, Eli Zaretskii wrote:
> We only need the function names, that's all.  A year from now, someone
> who will want to find out when this change was done in one of those
> functions, will grep the ChangeLog's for the function's name.

I don't think this is necessary, and it's definitely not customary.
Here's two of the examples from the GNU Coding Standards.  Re "Likewise":

* register.el (insert-register): Return nil.
(jump-to-register): Likewise.


And for large mechanical changes:

When you change the calling sequence of a function in a simple fashion,
and you change all the callers of the function to use the new calling
sequence, there is no need to make individual entries for all the
callers that you changed.  Just write in the entry for the function
being called, ``All callers changed''---like this:

@example
* keyboard.c (Fcommand_execute): New arg SPECIAL.
All callers changed.
@end example


-- 
Daniel Jacobowitz
CodeSourcery

Re: [5/9] Associate parsed condition with location

[Eli Zaretskii]

> Date: Sat, 8 Sep 2007 09:46:13 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <ghost@cs.msu.su>, gdb-patches@sources.redhat.com
> 
> On Sat, Sep 08, 2007 at 03:17:06PM +0300, Eli Zaretskii wrote:
> > We only need the function names, that's all.  A year from now, someone
> > who will want to find out when this change was done in one of those
> > functions, will grep the ChangeLog's for the function's name.
> 
> I don't think this is necessary, and it's definitely not customary.

It's never late to start making good log entries.

> Here's two of the examples from the GNU Coding Standards.  Re "Likewise":
> 
> * register.el (insert-register): Return nil.
> (jump-to-register): Likewise.
> 
> 
> And for large mechanical changes:
> 
> When you change the calling sequence of a function in a simple fashion,
> and you change all the callers of the function to use the new calling
> sequence, there is no need to make individual entries for all the
> callers that you changed.  Just write in the entry for the function
> being called, ``All callers changed''---like this:

Are we up to a quoting contest?  How about this one:

    It's important to name the changed function or variable in full.  Don't
    abbreviate function or variable names, and don't combine them.
    Subsequent maintainers will often search for a function name to find all
    the change log entries that pertain to it; if you abbreviate the name,
    they won't find it when they search.

Omitting function names is certainly not helpful when searching for
them in the logs.

Also, note the condition: ``when you change the calling sequence of a
function in a simple fashion''.  I don't think this is our case here.
The changes are not mechanical, either.

But I'm used to be voted down here lately; time for some
soul-searching (and this time of year is actually perfect for that, if
you know what I mean).

Re: [5/9] Associate parsed condition with location

[Daniel Jacobowitz]

On Sat, Sep 08, 2007 at 07:16:36PM +0300, Eli Zaretskii wrote:
> Also, note the condition: ``when you change the calling sequence of a
> function in a simple fashion''.  I don't think this is our case here.
> The changes are not mechanical, either.

If they're not mechanical, then I agree with you.  That's an issue of
personal taste; I hate writing repetative entries, so perhaps I have
an overly broad view of "mechanical" changes.

> (and this time of year is actually perfect for that, if
> you know what I mean).

Yes indeed.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [5/9] Associate parsed condition with location

[Vladimir Prus]

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

On Saturday 08 September 2007 15:11:40 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 8 Sep 2007 00:42:24 +0400
> > 
> > 	* breakpoint.c: Adjust.
> > 	* tui/tui-winsource.c: Adjust.
> 
> Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
> because they don't name the functions where the changes were made.

I've added the list of functions.

> 
> > --- gdb/breakpoint.h	(/work/mb_mainline/4_bpstat_owner)	(revision 4741)
> > +++ gdb/breakpoint.h	(/work/mb_mainline/5_per_loc_cond)	(revision 4741)
> > @@ -243,6 +243,13 @@ struct bp_location
> >       than reference counting.  */
> >    struct breakpoint *owner;
> >  
> > +  /* Conditional.  Break only if this expression's value is nonzero.  
> > +     Unlike string form of condition, which is associated with breakpoint,
> > +     this is associated with location, since if breakpoint has several
> > +     PC locations, the evaluation of expression can be different for
> > +     different PCs.  */
> > +  struct expression *cond;
> 
> I think we should use ``location'' instead of ``PC'', since you make
> the change for watchpoints as well, where the PC is irrelevant.

Changed.

Is the attached fine with you?

- Volodya


 



[-- Attachment #2: mainline_5_per_loc_cond.ChangeLog --]
[-- Type: text/plain, Size: 539 bytes --]

	gdb/
	* breakpoint.h (struct breakpoint): Move the cond 
	field to...
	(struct bp_location): Here.
	* breakpoint.c (condition_command, bpstat_stop_status,
	print_one_breakpoint, allocate_bp_location,
	solib_load_unload_1, create_fork_vfork_event_catchpoint,
	create_exec_event_catchpoint, create_breakpoints,
	break_command_1, watch_command_1, handle_gnu_v3_exceptions,
	create_ada_exception_breakpoint, set_breakpoint_sal,
	delete_breakpoint, breakpoint_re_set_one): Adjust.		
	* tui/tui-winsource.c (tui_update_breakpoint_info): Adjust.

[-- Attachment #3: mainline_5_per_loc_cond.diff --]
[-- Type: text/x-diff, Size: 9703 bytes --]

--- gdb/breakpoint.c	(/work/mb_mainline/4_bpstat_owner)	(revision 4828)
+++ gdb/breakpoint.c	(/work/mb_mainline/5_per_loc_cond)	(revision 4828)
@@ -569,17 +569,17 @@ condition_command (char *arg, int from_t
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
     {
-      if (b->cond)
+      struct bp_location *loc = b->loc;
+      if (loc->cond)
 	{
-	  xfree (b->cond);
-	  b->cond = 0;
+	  xfree (loc->cond);
+	  loc->cond = 0;
 	}
       if (b->cond_string != NULL)
 	xfree (b->cond_string);
 
       if (*p == 0)
 	{
-	  b->cond = 0;
 	  b->cond_string = NULL;
 	  if (from_tty)
 	    printf_filtered (_("Breakpoint %d now unconditional.\n"), bnum);
@@ -592,7 +592,8 @@ condition_command (char *arg, int from_t
 	  b->cond_string = savestring (arg, strlen (arg));
 	  if (!b->pending)
 	    {
-	      b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+	      b->loc->cond = parse_exp_1 (&arg, 
+					  block_for_pc (b->loc->address), 0);
 	      if (*arg)
 		error (_("Junk at end of expression"));
 	    }
@@ -2923,19 +2924,19 @@ bpstat_stop_status (CORE_ADDR bp_addr, p
       {
 	int value_is_zero = 0;
 
-	if (b->cond)
+	if (b->loc->cond)
 	  {
 	    /* Need to select the frame, with all that implies
 	       so that the conditions will have the right context.  */
 	    select_frame (get_current_frame ());
 	    value_is_zero
-	      = catch_errors (breakpoint_cond_eval, (b->cond),
+	      = catch_errors (breakpoint_cond_eval, (b->loc->cond),
 			      "Error in testing breakpoint condition:\n",
 			      RETURN_MASK_ALL);
 	    /* FIXME-someday, should give breakpoint # */
 	    free_all_values ();
 	  }
-	if (b->cond && value_is_zero)
+	if (b->loc->cond && value_is_zero)
 	  {
 	    bs->stop = 0;
 	    /* Don't consider this a hit.  */
@@ -3606,14 +3607,14 @@ print_one_breakpoint (struct breakpoint 
       ui_out_text (uiout, "\n");
     }
   
-  if (b->cond && !ada_exception_catchpoint_p (b))
+  if (b->loc->cond && !ada_exception_catchpoint_p (b))
     {
       /* We do not print the condition for Ada exception catchpoints
          because the condition is an internal implementation detail
          that we do not want to expose to the user.  */
       annotate_field (7);
       ui_out_text (uiout, "\tstop only if ");
-      print_expression (b->cond, stb->stream);
+      print_expression (b->loc->cond, stb->stream);
       ui_out_field_stream (uiout, "cond", stb);
       ui_out_text (uiout, "\n");
     }
@@ -4085,6 +4086,7 @@ allocate_bp_location (struct breakpoint 
   memset (loc, 0, sizeof (*loc));
 
   loc->owner = bpt;
+  loc->cond = NULL;
 
   switch (bp_type)
     {
@@ -4604,7 +4606,6 @@ solib_load_unload_1 (char *hookname, int
   b = set_raw_breakpoint (sals.sals[0], bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -4661,7 +4662,6 @@ create_fork_vfork_event_catchpoint (int 
   b = set_raw_breakpoint (sal, bp_kind);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -4700,7 +4700,6 @@ create_exec_event_catchpoint (int tempfl
   b = set_raw_breakpoint (sal, bp_catch_exec);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ?
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -5033,7 +5032,7 @@ create_breakpoints (struct symtabs_and_l
 	b = set_raw_breakpoint (sal, type);
 	set_breakpoint_count (breakpoint_count + 1);
 	b->number = breakpoint_count;
-	b->cond = cond[i];
+	b->loc->cond = cond[i];
 	b->thread = thread;
 	if (addr_string[i])
 	  b->addr_string = addr_string[i];
@@ -5056,7 +5055,7 @@ create_breakpoints (struct symtabs_and_l
 	      {
 		arg = pending_bp->cond_string;
 		b->cond_string = savestring (arg, strlen (arg));
-		b->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
+		b->loc->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
 		if (*arg)
 		  error (_("Junk at end of pending breakpoint condition expression"));
 	      }
@@ -5399,7 +5398,7 @@ break_command_1 (char *arg, int flag, in
 		              : bp_breakpoint);
       set_breakpoint_count (breakpoint_count + 1);
       b->number = breakpoint_count;
-      b->cond = *cond;
+      b->loc->cond = *cond;
       b->thread = thread;
       b->addr_string = *addr_string;
       b->cond_string = *cond_string;
@@ -5792,7 +5791,7 @@ watch_command_1 (char *arg, int accessfl
   b->exp_valid_block = exp_valid_block;
   b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
-  b->cond = cond;
+  b->loc->cond = cond;
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
@@ -6352,7 +6351,6 @@ create_exception_catchpoint (int tempfla
   b = set_raw_breakpoint (*sal, bptype);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = thread;
@@ -6432,7 +6430,6 @@ handle_gnu_v3_exceptions (int tempflag, 
   b = set_raw_breakpoint (sals.sals[0], bp_breakpoint);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = NULL;
   b->cond_string = (cond_string == NULL) ? 
     NULL : savestring (cond_string, strlen (cond_string));
   b->thread = -1;
@@ -6519,7 +6516,7 @@ create_ada_exception_breakpoint (struct 
   b->disposition = tempflag ? disp_del : disp_donttouch;
   b->number = breakpoint_count;
   b->ignore_count = 0;
-  b->cond = cond;
+  b->loc->cond = cond;
   b->addr_string = addr_string;
   b->language = language_ada;
   b->cond_string = cond_string;
@@ -6701,7 +6698,6 @@ set_breakpoint_sal (struct symtab_and_li
   b = set_raw_breakpoint (sal, bp_breakpoint);
   set_breakpoint_count (breakpoint_count + 1);
   b->number = breakpoint_count;
-  b->cond = 0;
   b->thread = -1;
   return b;
 }
@@ -7021,8 +7017,6 @@ delete_breakpoint (struct breakpoint *bp
     }
 
   free_command_lines (&bpt->commands);
-  if (bpt->cond)
-    xfree (bpt->cond);
   if (bpt->cond_string != NULL)
     xfree (bpt->cond_string);
   if (bpt->addr_string != NULL)
@@ -7062,6 +7056,8 @@ delete_breakpoint (struct breakpoint *bp
      bp, we mark it as deleted before freeing its storage. */
   bpt->type = bp_none;
 
+  if (bpt->loc->cond)
+    xfree (bpt->loc->cond);
   xfree (bpt->loc);
   xfree (bpt);
 }
@@ -7215,14 +7211,14 @@ breakpoint_re_set_one (void *bint)
 	  if (b->cond_string != NULL)
 	    {
 	      s = b->cond_string;
-	      if (b->cond)
+	      if (b->loc->cond)
 		{
-		  xfree (b->cond);
+		  xfree (b->loc->cond);
 		  /* Avoid re-freeing b->exp if an error during the call
 		     to parse_exp_1.  */
-		  b->cond = NULL;
+		  b->loc->cond = NULL;
 		}
-	      b->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 0);
+	      b->loc->cond = parse_exp_1 (&s, block_for_pc (sals.sals[i].pc), 0);
 	    }
 
 	  /* We need to re-set the breakpoint if the address changes... */
@@ -7314,14 +7310,14 @@ breakpoint_re_set_one (void *bint)
       if (b->cond_string != NULL)
 	{
 	  s = b->cond_string;
-	  if (b->cond)
+	  if (b->loc->cond)
 	    {
-	      xfree (b->cond);
+	      xfree (b->loc->cond);
 	      /* Avoid re-freeing b->exp if an error during the call
 		 to parse_exp_1.  */
-	      b->cond = NULL;
+	      b->loc->cond = NULL;
 	    }
-	  b->cond = parse_exp_1 (&s, (struct block *) 0, 0);
+	  b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0);
 	}
       if (breakpoint_enabled (b))
 	mention (b);
--- gdb/breakpoint.h	(/work/mb_mainline/4_bpstat_owner)	(revision 4828)
+++ gdb/breakpoint.h	(/work/mb_mainline/5_per_loc_cond)	(revision 4828)
@@ -243,6 +243,13 @@ struct bp_location
      than reference counting.  */
   struct breakpoint *owner;
 
+  /* Conditional.  Break only if this expression's value is nonzero.  
+     Unlike string form of condition, which is associated with breakpoint,
+     this is associated with location, since if breakpoint has several
+     locations,  the evaluation of expression can be different for
+     different locations.  */
+  struct expression *cond;
+  
   /* Nonzero if this breakpoint is now inserted.  */
   char inserted;
 
@@ -341,8 +348,6 @@ struct breakpoint
     /* Stack depth (address of frame).  If nonzero, break only if fp
        equals this.  */
     struct frame_id frame_id;
-    /* Conditional.  Break only if this expression's value is nonzero.  */
-    struct expression *cond;
 
     /* String we used to set the breakpoint (malloc'd).  */
     char *addr_string;
--- gdb/tui/tui-winsource.c	(/work/mb_mainline/4_bpstat_owner)	(revision 4828)
+++ gdb/tui/tui-winsource.c	(/work/mb_mainline/5_per_loc_cond)	(revision 4828)
@@ -449,7 +449,7 @@ tui_update_breakpoint_info (struct tui_w
                 mode |= TUI_BP_ENABLED;
               if (bp->hit_count)
                 mode |= TUI_BP_HIT;
-              if (bp->cond)
+              if (bp->loc->cond)
                 mode |= TUI_BP_CONDITIONAL;
               if (bp->type == bp_hardware_breakpoint)
                 mode |= TUI_BP_HARDWARE;

Property changes on: 
___________________________________________________________________
Name: svk:merge
 +d48a11ec-ee1c-0410-b3f5-c20844f99675:/work/mb_mainline/4_bpstat_owner:4823
  e7755896-6108-0410-9592-8049d3e74e28:/mirrors/gdb/trunk:182811

Re: [5/9] Associate parsed condition with location

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Sat, 22 Sep 2007 21:53:22 +0400
> Cc: gdb-patches@sources.redhat.com
> 
> 
> On Saturday 08 September 2007 15:11:40 Eli Zaretskii wrote:
> > > From: Vladimir Prus <vladimir@codesourcery.com>
> > > Date: Sat, 8 Sep 2007 00:42:24 +0400
> > > 
> > > 	* breakpoint.c: Adjust.
> > > 	* tui/tui-winsource.c: Adjust.
> > 
> > Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
> > because they don't name the functions where the changes were made.
> 
> I've added the list of functions.

Thanks.  However:

> 	* breakpoint.c (condition_command, bpstat_stop_status,
> 	print_one_breakpoint, allocate_bp_location,
> 	solib_load_unload_1, create_fork_vfork_event_catchpoint,
> 	create_exec_event_catchpoint, create_breakpoints,
> 	break_command_1, watch_command_1, handle_gnu_v3_exceptions,
> 	create_ada_exception_breakpoint, set_breakpoint_sal,
> 	delete_breakpoint, breakpoint_re_set_one): Adjust.		

When a list of functions is longer than one line, it should be
formatted like this:

 	* breakpoint.c (condition_command, bpstat_stop_status)
 	(print_one_breakpoint, allocate_bp_location)
 	(solib_load_unload_1, create_fork_vfork_event_catchpoint)
 	(create_exec_event_catchpoint, create_breakpoints)
 	(break_command_1, watch_command_1, handle_gnu_v3_exceptions)
 	(create_ada_exception_breakpoint, set_breakpoint_sal)
 	(delete_breakpoint, breakpoint_re_set_one): Adjust.		

(the reason is to help Emacs highlight the function names correctly).
Believe it or not, but this is in the GNU Coding Standards.

> > I think we should use ``location'' instead of ``PC'', since you make
> > the change for watchpoints as well, where the PC is irrelevant.
> 
> Changed.
> 
> Is the attached fine with you?

Yes.  Thanks.

Re: [5/9] Associate parsed condition with location

[Vladimir Prus]

On Saturday 22 September 2007 22:33:33 Eli Zaretskii wrote:
> > From: Vladimir Prus <vladimir@codesourcery.com>
> > Date: Sat, 22 Sep 2007 21:53:22 +0400
> > Cc: gdb-patches@sources.redhat.com
> > 
> > 
> > On Saturday 08 September 2007 15:11:40 Eli Zaretskii wrote:
> > > > From: Vladimir Prus <vladimir@codesourcery.com>
> > > > Date: Sat, 8 Sep 2007 00:42:24 +0400
> > > > 
> > > > 	* breakpoint.c: Adjust.
> > > > 	* tui/tui-winsource.c: Adjust.
> > > 
> > > Nitpicking: these aren't, strictly speaking, valid ChangeLog entries,
> > > because they don't name the functions where the changes were made.
> > 
> > I've added the list of functions.
> 
> Thanks.  However:
> 
> > 	* breakpoint.c (condition_command, bpstat_stop_status,
> > 	print_one_breakpoint, allocate_bp_location,
> > 	solib_load_unload_1, create_fork_vfork_event_catchpoint,
> > 	create_exec_event_catchpoint, create_breakpoints,
> > 	break_command_1, watch_command_1, handle_gnu_v3_exceptions,
> > 	create_ada_exception_breakpoint, set_breakpoint_sal,
> > 	delete_breakpoint, breakpoint_re_set_one): Adjust.		
> 
> When a list of functions is longer than one line, it should be
> formatted like this:
> 
>  	* breakpoint.c (condition_command, bpstat_stop_status)
>  	(print_one_breakpoint, allocate_bp_location)
>  	(solib_load_unload_1, create_fork_vfork_event_catchpoint)
>  	(create_exec_event_catchpoint, create_breakpoints)
>  	(break_command_1, watch_command_1, handle_gnu_v3_exceptions)
>  	(create_ada_exception_breakpoint, set_breakpoint_sal)
>  	(delete_breakpoint, breakpoint_re_set_one): Adjust.		
> 
> (the reason is to help Emacs highlight the function names correctly).
> Believe it or not, but this is in the GNU Coding Standards.

Ehm. I always though that it's editor that should adjust to coding
conventions, not the other way around, but I'm not going to argue
this point.

I'll adjust and commit.

Thanks,
Volodya