Daniel Jacobowitz wrote:

>On Tue, Jan 27, 2004 at 03:41:04PM -0500, J. Johnston wrote:
>  
>
>>Anyway, I have a new and improved patch attached that 
>>addresses the issues below.  I have included answers to questions below as 
>>well.
>>    
>>
>
>Thanks.  I have just one question and a couple of cosmetic issues left.
>
>I'm also still curious about pending breakpoints which resolve to
>multiple functions, et cetera.  We can revisit at a later date.
>  
>
Ok, thanks.

>  
>
>>>When reading the testsuite diffs I asked:
>>> Might deferred be a better name for this concept anyway?
>>>
>>>Comments?
>>>
>>>      
>>>
>>This is a case of "you can't please everybody all the time".  I am 
>>certainly happy with it.
>>    
>>
>
>OK.  I have some ideas for better wording of the message, but I can
>take care of it later.
>  
>
Ok.

>  
>
>>Well, it only gets there on a "not-found" error from decode_line_1 so the 
>>onus is on decode_line_1 and it's children to do proper cleanups which is 
>>outside of this patch.
>>    
>>
>
>Gotcha.
>
>About the new patch:
>
>  
>
>>+    /* Flag value for pending breakpoint.
>>+       first bit  : 0 non-temporary, 1 temporary.
>>+       second bit : 0 normal breakpoint, 1 hardware breakpoint. */
>>+    int flag;
>>    
>>
>
>OK, now I can see what flag is used for.  This raises a new question. 
>Should temporary breakpoints be allowed to be pending?
>
>Oh, I guess the answer is yes.  tbreak my_dso_initialization; run.  So
>this is fine.
>  
>
Great.

>  
>
>>+/* Try and resolve a pending breakpoint.  */
>>+static int
>>+resolve_pending_breakpoint (struct breakpoint *b)
>>    
>>
>
>I love the new version of this function...
>
>  
>
>>+  input_radix = b->input_radix;
>>+  rc = break_command_1 (b->addr_string, b->flag, b->from_tty, b);
>>+  
>>+  if (rc == GDB_RC_OK)
>>+    /* Pending breakpoint has been resolved.  */
>>+    printf_filtered ("Resolves pending breakpoint \"%s\"\n", b->addr_string);
>>    
>>
>
>Spelling error, though.
>  
>
Actually, I meant it to be:

Breakpoint set at.....
Resolves pending breakpoint "....."

So the bottom line ties to the breakpoint just set.  If this isn't very 
clear I can put "which" in front or just make it "Resolved" which I 
think you are alluding to.  If there is some other spelling error, 
please point it out as I don't see it.

>  
>
>>@@ -4779,6 +4888,26 @@ create_breakpoints (struct symtabs_and_l
>> 	b->ignore_count = ignore_count;
>> 	b->enable_state = bp_enabled;
>> 	b->disposition = disposition;
>>+	/* If resolving a pending breakpoint, a check must be made to see if
>>+	   the user has specified a new condition or commands for the 
>>+	   breakpoint.  A new condition will override any condition that was 
>>+	   initially specified with the initial breakpoint command.  */
>>+	if (pending_bp)
>>+	  {
>>+	    char *arg;
>>+	    if (pending_bp->cond_string)
>>+	      {
>>+		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);
>>+		if (*arg)
>>+		  error ("Junk at end of pending breakpoint condition expression");
>>+	      }
>>+	    /* If there are commands associated with the breakpoint, they should 
>>+	       be copied too.  */
>>+	    if (pending_bp->commands)
>>+	      b->commands = copy_command_lines (pending_bp->commands);
>>+	  }
>> 	mention (b);
>>       }
>>   }    
>>    
>>
>
>Here's the one question.
>
>The only way to get here with a PENDING_BP is from break_command_1 from
>resolve_pending_breakpoint.  So I don't think it is possible for there
>to be a condition already set on B, which makes the comment about
>"overriding" such a condition a little strange.  Am I right, or is
>there some other way to get a condition to here?
>  
>
The scenario would be:  1. User creates a pending breakpoint with a 
condition in the break location.  2. User specifies a condition for the 
breakpoint number given back for the pending breakpoint using the 
condition command.  3. The shared library gets loaded that resolves the 
breakpoint.  The resolution of the breakpoint will find the original 
condition in the location string, but won't know about the 2nd one which 
gets stored in the pending breakpoint cond_string (see condition_command 
support for pending breakpoint).

>  
>
>>-static void
>>-break_command_1 (char *arg, int flag, int from_tty)
>>+   PENDING_BP is non-NULL when this function is being called to resolve
>>+   a pending breakpoint.  */
>>+
>>+static int
>>+break_command_1 (char *arg, int flag, int from_tty, struct breakpoint *pending_bp)
>> {
>>   int tempflag, hardwareflag;
>>   struct symtabs_and_lines sals;
>>   struct expression **cond = 0;
>>+  struct symtab_and_line pending_sal;
>>   /* Pointers in arg to the start, and one past the end, of the
>>      condition.  */
>>    
>>
>
>Aha!  That's where that baffling comment came from, it was already
>present in break_command_1.  Here we have a var named ARG, so it makes
>more sense, but the variables cond_start and cond_end that it refers to
>have walked down about a hundred lines.
>  
>
I will remove this comment in break_command_1 too.

>  
>
>>+  if (bpt->pending)
>>+    {
>>+      if (bpt->enable_state != bp_enabled)
>>+	{
>>+	  /* When enabling a pending breakpoint, we need to check if the breakpoint
>>+	     is resolvable since shared libraries could have been loaded
>>+	     after the breakpoint was disabled.  */
>>+	  struct breakpoint *new_bp;
>>+	  breakpoints_changed ();
>>+ 	  if (resolve_pending_breakpoint (bpt) == GDB_RC_OK)
>>    
>>
>
>new_bp is no longer used, you can remove it.
>  
>
Ok, fixed.

I have attached an updated patch.  Ok to commit?

-- Jeff J.