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

[Daniel Jacobowitz <drow at mvista dot com>]

On Wed, Jan 21, 2004 at 03:52:42PM -0500, Jeff Johnston wrote:
> This is the first of the last 3 patches for pending breakpoint support.
> This patch adds the actual support.  The other two patches are the
> documentation (previously approved, but before code was accepted) and
> changes to the testsuite.
> 
> This patch accounts for previous comments on the breakpoint code.
> 
> Ok to commit?

Sorry, but a number of my comments from last time still apply, and I
have some others.  The concept is looking great now, though!

> Index: breakpoint.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.h,v
> retrieving revision 1.26
> diff -u -p -r1.26 breakpoint.h
> --- breakpoint.h	6 Nov 2003 18:24:55 -0000	1.26
> +++ breakpoint.h	21 Jan 2004 02:34:10 -0000
> @@ -385,6 +385,15 @@ struct breakpoint
>  
>      /* Methods associated with this breakpoint.  */
>      struct breakpoint_ops *ops;
> +
> +    /* Initial from_tty value.  */
> +    int from_tty;
> +
> +    /* Initial flag value.  */
> +    int flag;
> +
> +    /* Is breakpoint pending on shlib loads?  */
> +    int pending;

Can you read this comment and figure out from it what flag is, or even
where to look for more information?  I can't.  Please indicate where
they come from, and that they are used for pending breakpoints. 
Something more descriptive than "flag" would be good.

> @@ -3458,7 +3463,15 @@ print_one_breakpoint (struct breakpoint 
>  	if (addressprint)
>  	  {
>  	    annotate_field (4);
> -	    ui_out_field_core_addr (uiout, "addr", b->loc->address);
> +	    if (b->pending)
> +	      {
> +		if (TARGET_ADDR_BIT <= 32)
> +		  ui_out_field_string (uiout, "addr", "<PENDING>  ");
> +		else
> +		  ui_out_field_string (uiout, "addr", "<PENDING>        ");
> +	      }
> +	    else
> +	      ui_out_field_core_addr (uiout, "addr", b->loc->address);
>  	  }
>  	annotate_field (5);
>  	*last_addr = b->loc->address;

Last time I said:

  I'm curious what effect this will have on MI consumers.  Also, the
  resulting MI output will be somewhat gruesome with the embedded spaces.
  Maybe:
        ui_out_field_string (uiout, "addr", "<PENDING>");
        ui_out_text (uiout, "addr", "  ");
  but I'm not sure that works without testing it.

  The other possible alternative is not outputting addr at all for
  MI-like uiouts.  May be better to do this (more flexible).

I still don't know what to do about the latter, but please try the
former.

> @@ -4288,23 +4316,139 @@ disable_breakpoints_in_shlibs (int silen
>    }
>  }
>  
> +struct captured_parse_breakpoint_args
> +  {
> +    char **arg_p;
> +    struct symtabs_and_lines *sals_p;
> +    char ***addr_string_p;
> +    int *not_found_ptr;
> +  };
> +
> +struct lang_and_radix
> +  {
> +    enum language lang;
> +    int radix;
> +  };
> +
> +/* Cleanup helper routine to restore the current language and
> +   input radix.  */
> +static void
> +do_restore_lang_radix_cleanup (void *old)
> +{
> +  struct lang_and_radix *p = old;
> +  set_language (p->lang);
> +  input_radix = p->radix;
> +}
> +
> +/* Try and resolve a pending breakpoint.  */
> +static struct breakpoint *
> +resolve_pending_breakpoint (struct breakpoint *b)
> +{
> +  /* Try and reparse the breakpoint in case the shared library
> +     is now loaded.  */
> +  struct symtabs_and_lines sals;
> +  struct symtab_and_line pending_sal;
> +  /* Pointers in arg to the start, and one past the end, of the
> +     condition.  */

Last time I read this patch I read this comment and said "Huh?"  That
still applies :) What pointers in what arg to the end of what
condition?

> +  char **cond_string = (char **) NULL;
> +  char *copy_arg = b->addr_string;
> +  char **addr_string;
> +  char *errmsg;
> +  struct captured_parse_breakpoint_args parse_args;
> +  int rc;
> +  int not_found = 0;
> +  struct ui_file *old_gdb_stderr;
> +  
> +  sals.sals = NULL;
> +  sals.nelts = 0;
> +  addr_string = NULL;
> +  
> +  parse_args.arg_p = &copy_arg;
> +  parse_args.sals_p = &sals;
> +  parse_args.addr_string_p = &addr_string;
> +  parse_args.not_found_ptr = &not_found;
> +  
> +  rc = catch_exceptions (uiout, do_captured_parse_breakpoint, 
> +		         &parse_args, NULL, RETURN_MASK_ALL);
> +  
> +  if (rc == GDB_RC_OK)
> +    {
> +      struct lang_and_radix old_lr;
> +      struct cleanup *old_chain;
> +      char *arg;
> +      struct breakpoint *b1;
> +      
> +      printf_filtered ("Pending breakpoint \"%s\" resolved\n", b->addr_string);
> +      
> +      /* Set language, input-radix, then reissue breakpoint command. 
> +	 Ensure the language and input-radix are restored afterwards.  */
> +      old_lr.lang = current_language->la_language;
> +      old_lr.radix = input_radix;
> +      old_chain = make_cleanup (do_restore_lang_radix_cleanup, &old_lr);

Thanks for adding the cleanup.

> +      
> +      set_language (b->language);
> +      input_radix = b->input_radix;
> +      break_command_1 (b->addr_string, b->flag, b->from_tty);
> +      b1 = breakpoint_chain;
> +      while (b1->next)
> +	b1 = b1->next;

I would really prefer that you not walk the breakpoint chain looking
for the last breakpoint.

> +      /* If there is condition specified, it should be copied over.  */
> +      if (b->cond_string)
> +	{
> +	  arg = b->cond_string;
> +	  b1->cond_string = savestring (arg, strlen (arg));
> +	  b1->cond = parse_exp_1 (&arg, block_for_pc (b->loc->address), 0);
> +	  if (*arg)
> +	    error ("Junk at end of expression");
> +	}
> +      /* If there are commands associated with the breakpoint, they should 
> +         be copied too.  */
> +      if (b->commands)
> +	{
> +	  b1->commands = copy_command_lines (b->commands);
> +	}

No, I've already asked you to do entire section differently:
  This, on the other hand, is not OK.  First of all you're wasting work -
  you parse the breakpoint location twice.  Then you go grubbing around
  in the breakpoint chain looking for it, which assumes it will be added
  at the end of the chain - it will, but that sort of detail shouldn't be
  exposed.  And break_command_1 can currently create multiple
  breakpoints, so you'll get that wrong too.
  
  decode_line_1 can end up prompting the user; you should have a testcase
  for that and make sure it does something sane.  Easiest way is probably
  to put it in gdb.cp using an overloaded function.  All set breakpoints
  should get the condition and commands.
  
  Can you arrange for most of this function to happen inside
  break_command_1, possibly by giving it a pending breakpoint as an
  optional argument?  Other ideas?

Also, I'm 99% sure you mean b1->loc->address above.  b->loc->address is
the pending breakpoint's address, which is unset.

> +
> +      do_cleanups (old_chain);
> +      return b1; /* Pending breakpoint resolved.  */
> +    }
> +
> +  /* Otherwise, we didn't successfully resolve pending breakpoint.  */
> +  return NULL;
> +}
> +
>  /* Try to reenable any breakpoints in shared libraries.  */
>  void
>  re_enable_breakpoints_in_shlibs (void)
>  {
>    struct breakpoint *b;
> +  struct breakpoint *del_b = NULL;
>  
>    ALL_BREAKPOINTS (b)
> +  {
> +    if (del_b)
> +      {
> +	delete_breakpoint (del_b);
> +	del_b = NULL;
> +      }

I said:
  No, use ALL_BREAKPOINTS_SAFE and you can get rid of del_b.

> @@ -4923,19 +5094,51 @@ break_command_1 (char *arg, int flag, in
>    sals.sals = NULL;
>    sals.nelts = 0;
>    addr_string = NULL;
> -  parse_breakpoint_sals (&arg, &sals, &addr_string);
>  
> -  if (!sals.nelts)
> +  parse_args.arg_p = &arg;
> +  parse_args.sals_p = &sals;
> +  parse_args.addr_string_p = &addr_string;
> +  parse_args.not_found_ptr = &not_found;
> +
> +  rc = catch_exceptions_with_msg (uiout, do_captured_parse_breakpoint, 
> +		  		  &parse_args, NULL, &err_msg, 
> +				  RETURN_MASK_ALL);

Awesome, the need to play with the text of the error message is gone.

> +
> +  if (rc != GDB_RC_OK)
> +    {
> +      /* Check for file or function not found.  */
> +      if (not_found)
> +	{
> +	  error_output_message (NULL, err_msg);
> +	  xfree (err_msg);
> +	  if (!query ("Make breakpoint pending on future shared library load? "))
> +	    return;

I said:
  I'm still not really happy with this wording.  What does that mean?
  Plus, it'll look really out of place if we don't have shared library
  support.

When reading the testsuite diffs I asked:
  Might deferred be a better name for this concept anyway?

Comments?

> +	  copy_arg = (char *)xmalloc (strlen (addr_start));
> +	  strcpy (copy_arg, addr_start);

I said:
  The cast is not necessary nowadays.  There are also a number of
  functions for doing this, which don't suffer from the off-by-one error
  above :)  Try xstrdup instead.

> +	  addr_string = &copy_arg;

I said:
  Is addr_string guaranteed to still be NULL at this point, or are we
  leaking?

The answer to this question is somewhere in the depths of
decode_line_1, so I'm not sure what the answer is.

> +	  sals.nelts = 1;
> +	  sals.sals = &pending_sal;
> +	  pending_sal.pc = 0;
> +	  pending = 1;
> +	}
> +      else
> +	return;
> +    }
> +  else if (!sals.nelts)
>      return;
>  
> +
>    /* Create a chain of things that always need to be cleaned up. */
>    old_chain = make_cleanup (null_cleanup, 0);
>  
> -  /* Make sure that all storage allocated to SALS gets freed.  */
> -  make_cleanup (xfree, sals.sals);
> -
> -  /* Cleanup the addr_string array but not its contents. */
> -  make_cleanup (xfree, addr_string);
> +  if (!pending)
> +    {
> +      /* Make sure that all storage allocated to SALS gets freed.  */
> +      make_cleanup (xfree, sals.sals);
> +      
> +      /* Cleanup the addr_string array but not its contents. */
> +      make_cleanup (xfree, addr_string);
> +    }
>  
>    /* Allocate space for all the cond expressions. */
>    cond = xcalloc (sals.nelts, sizeof (struct expression *));

Yes, you don't want to free addr_string, but a few lines further down
you probably want to add a cleanup to free copy_arg on the
breakpoint_chain.

> @@ -7363,70 +7600,92 @@ do_enable_breakpoint (struct breakpoint 
>  	error ("Hardware breakpoints used exceeds limit.");
>      }
>  
> -  if (bpt->enable_state != bp_permanent)
> -    bpt->enable_state = bp_enabled;
> -  bpt->disposition = disposition;
> -  check_duplicates (bpt);
> -  breakpoints_changed ();
> -
> -  if (bpt->type == bp_watchpoint || 
> -      bpt->type == bp_hardware_watchpoint ||
> -      bpt->type == bp_read_watchpoint || 
> -      bpt->type == bp_access_watchpoint)
> -    {
> -      if (bpt->exp_valid_block != NULL)
> -	{
> -	  struct frame_info *fr =
> -	  fr = frame_find_by_id (bpt->watchpoint_frame);
> -	  if (fr == NULL)
> +  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 ();

I said:
  Sure this is necessary?  If we resolve the breakpoint successfully,
  this will get called from set_raw_breakpoint.

> + 	  if ((new_bp = resolve_pending_breakpoint (bpt)) != NULL)

I said:
  Might as well not use assignment-in-if-statement in new code.

I think this is in the ARI.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer