Re: [RFA]: pending breakpoint support [1/3]

[Daniel Jacobowitz <drow at mvista dot com>]

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.

> >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.

> 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.

> +/* 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.

> @@ -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?

> -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.

> +  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.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer