RFA: implement ambiguous linespec proposal

Yao Qi yao@codesourcery.com
Wed Nov 16 08:15:00 GMT 2011


On 11/15/2011 05:10 AM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> Here is a refresh of this patch.  This fixes the regressions noted by
> Tom> Jan, but also changes ovsrch.exp not to assume that namespace lookups
> Tom> are done.
> 
> Here is the final revision.
> 
> I plan to commit this sometime this week, barring objections or
> comments; after the doc patch (forthcoming) is approved.
>

Thanks to Doug, I applied your patch on GDB CVS 2011-11-10.  My comments
below,

>  
> @@ -7579,15 +7438,20 @@ check_fast_tracepoint_sals (struct gdbarch *gdbarch,
>  
>    for (i = 0; i < sals->nelts; i++)
>      {
> +      struct gdbarch *sarch;
> +
>        sal = &sals->sals[i];
>  
> -      rslt = gdbarch_fast_tracepoint_valid_at (gdbarch, sal->pc,
> +      sarch = get_sal_arch (*sal);

I suggest that we add a comment, I steal from Ulrich's comment :)

	 /* We fall back to GDBARCH if there is no architecture
 	    associated with SAL.  */

> +      if (sarch == NULL)
> +	sarch = gdbarch;
> +      rslt = gdbarch_fast_tracepoint_valid_at (sarch, sal->pc,
>  					       NULL, &msg);
>        old_chain = make_cleanup (xfree, msg);
>  
>        if (!rslt)
>  	error (_("May not have a fast tracepoint at 0x%s%s"),
> -	       paddress (gdbarch, sal->pc), (msg ? msg : ""));
> +	       paddress (sarch, sal->pc), (msg ? msg : ""));
>  
>        do_cleanups (old_chain);
>      }
> @@ -7729,8 +7593,6 @@ create_breakpoint (struct gdbarch *gdbarch,
>  		   int from_tty, int enabled, int internal)
>  {
>    volatile struct gdb_exception e;
> -  struct symtabs_and_lines sals;
> -  struct symtab_and_line pending_sal;
>    char *copy_arg;

I got a compilation warning below, and looks like copy_arg should be
initialized to NULL.

gdb/breakpoint.c:7636: error: 'copy_arg' may be used uninitialized
in this functiongdb/breakpoint.c:7636: error: 'copy_arg' may be used
uninitialized in this function

>    char *addr_start = arg;
>    struct linespec_result canonical;
> @@ -7743,26 +7605,26 @@ create_breakpoint (struct gdbarch *gdbarch,
>  
>    gdb_assert (ops != NULL);
>  
> -  sals.sals = NULL;
> -  sals.nelts = 0;
>    init_linespec_result (&canonical);
>  
>    if (type_wanted == bp_static_tracepoint && is_marker_spec (arg))
>      {
>        int i;
> +      struct linespec_sals lsal;
>  
> -      sals = decode_static_tracepoint_spec (&arg);
> +      lsal.sals = decode_static_tracepoint_spec (&arg);
>  
>        copy_arg = savestring (addr_start, arg - addr_start);
> -      canonical.canonical = xcalloc (sals.nelts, sizeof (char *));
> -      for (i = 0; i < sals.nelts; i++)
> -	canonical.canonical[i] = xstrdup (copy_arg);
> +
> +      lsal.canonical = xstrdup (copy_arg);
> +      VEC_safe_push (linespec_sals, canonical.sals, &lsal);
> +
>        goto done;
>      }
>  
>    TRY_CATCH (e, RETURN_MASK_ALL)
>      {
> -      parse_breakpoint_sals (&arg, &sals, &canonical);
> +      parse_breakpoint_sals (&arg, &canonical);
>      }
>  
>    /* If caller is interested in rc value from parse, set value.  */
> @@ -7794,35 +7656,31 @@ create_breakpoint (struct gdbarch *gdbarch,
>  	     a pending breakpoint and selected yes, or pending
>  	     breakpoint behavior is on and thus a pending breakpoint
>  	     is defaulted on behalf of the user.  */
> -	  copy_arg = xstrdup (addr_start);
> -	  canonical.canonical = &copy_arg;
> -	  sals.nelts = 1;
> -	  sals.sals = &pending_sal;
> -	  pending_sal.pc = 0;
> -	  pending = 1;
> +	  {
> +	    struct linespec_sals lsal;
> +
> +	    copy_arg = xstrdup (addr_start);
> +	    lsal.canonical = xstrdup (copy_arg);
> +	    lsal.sals.nelts = 1;
> +	    lsal.sals.sals = XNEW (struct symtab_and_line);
> +	    init_sal (&lsal.sals.sals[0]);
> +	    pending = 1;
> +	    VEC_safe_push (linespec_sals, canonical.sals, &lsal);
> +	  }
>  	  break;
>  	default:
>  	  throw_exception (e);
>  	}
>        break;
>      default:
> -      if (!sals.nelts)
> +      if (VEC_empty (linespec_sals, canonical.sals))
>  	return 0;
>      }
>  
>    done:
>  
>    /* Create a chain of things that always need to be cleaned up.  */
> -  old_chain = make_cleanup (null_cleanup, 0);
> -
> -  if (!pending)
> -    {
> -      /* Make sure that all storage allocated to SALS gets freed.  */
> -      make_cleanup (xfree, sals.sals);
> -      
> -      /* Cleanup the canonical array but not its contents.  */
> -      make_cleanup (xfree, canonical.canonical);
> -    }
> +  old_chain = make_cleanup_destroy_linespec_result (&canonical);
>  
>    /* ----------------------------- SNIP -----------------------------
>       Anything added to the cleanup chain beyond this point is assumed
> @@ -7830,28 +7688,36 @@ create_breakpoint (struct gdbarch *gdbarch,
>       then the memory is not reclaimed.  */
>    bkpt_chain = make_cleanup (null_cleanup, 0);
>  
> -  /* Mark the contents of the canonical for cleanup.  These go on
> -     the bkpt_chain and only occur if the breakpoint create fails.  */
> -  for (i = 0; i < sals.nelts; i++)
> -    {
> -      if (canonical.canonical[i] != NULL)
> -	make_cleanup (xfree, canonical.canonical[i]);
> -    }
> -
>    /* Resolve all line numbers to PC's and verify that the addresses
>       are ok for the target.  */
>    if (!pending)
> -    breakpoint_sals_to_pc (&sals);
> +    {
> +      int ix;
> +      struct linespec_sals *iter;
> +
> +      for (ix = 0; VEC_iterate (linespec_sals, canonical.sals, ix, iter); ++ix)
> +	breakpoint_sals_to_pc (&iter->sals);
> +    }
>  
>    /* Fast tracepoints may have additional restrictions on location.  */
>    if (type_wanted == bp_fast_tracepoint)
> -    check_fast_tracepoint_sals (gdbarch, &sals);
> +    {
> +      int ix;
> +      struct linespec_sals *iter;
> +
> +      for (ix = 0; VEC_iterate (linespec_sals, canonical.sals, ix, iter); ++ix)
> +	check_fast_tracepoint_sals (gdbarch, &iter->sals);
> +    }
>  
>    /* Verify that condition can be parsed, before setting any
>       breakpoints.  Allocate a separate condition expression for each
>       breakpoint.  */
>    if (!pending)
>      {
> +      struct linespec_sals *lsal;
> +
> +      lsal = VEC_index (linespec_sals, canonical.sals, 0);
> +
>        if (parse_condition_and_thread)
>          {
>              /* Here we only parse 'arg' to separate condition
> @@ -7860,7 +7726,7 @@ create_breakpoint (struct gdbarch *gdbarch,
>                 re-parse it in context of each sal.  */
>              cond_string = NULL;
>              thread = -1;
> -            find_condition_and_thread (arg, sals.sals[0].pc, &cond_string,
> +            find_condition_and_thread (arg, lsal->sals.sals[0].pc, &cond_string,
>                                         &thread, &task);
>              if (cond_string)
>                  make_cleanup (xfree, cond_string);
> @@ -7882,24 +7748,26 @@ create_breakpoint (struct gdbarch *gdbarch,
>  	 expand multiple locations for each sal, given than SALS
>  	 already should contain all sals for MARKER_ID.  */
>        if (type_wanted == bp_static_tracepoint
> -	  && is_marker_spec (canonical.canonical[0]))
> +	  && is_marker_spec (lsal->canonical))
>  	{
>  	  int i;
>  
> -	  for (i = 0; i < sals.nelts; ++i)
> +	  for (i = 0; i < lsal->sals.nelts; ++i)
>  	    {
>  	      struct symtabs_and_lines expanded;
>  	      struct tracepoint *tp;
>  	      struct cleanup *old_chain;
> +	      char *addr_string;
>  
>  	      expanded.nelts = 1;
> -	      expanded.sals = xmalloc (sizeof (struct symtab_and_line));
> -	      expanded.sals[0] = sals.sals[i];
> -	      old_chain = make_cleanup (xfree, expanded.sals);
> +	      expanded.sals = &lsal->sals.sals[i];
> +
> +	      addr_string = xstrdup (canonical.addr_string);
> +	      old_chain = make_cleanup (xfree, addr_string);
>  
>  	      tp = XCNEW (struct tracepoint);
>  	      init_breakpoint_sal (&tp->base, gdbarch, expanded,
> -				   canonical.canonical[i],
> +				   addr_string, NULL,
>  				   cond_string, type_wanted,
>  				   tempflag ? disp_del : disp_donttouch,
>  				   thread, task, ignore_count, ops,
> @@ -11619,8 +11488,17 @@ update_breakpoint_locations (struct breakpoint *b,
>    int i;
>    struct bp_location *existing_locations = b->loc;
>  
> -  /* Ranged breakpoints have only one start location and one end location.  */
> -  gdb_assert (sals_end.nelts == 0 || (sals.nelts == 1 && sals_end.nelts == 1));
> +  if (sals_end.nelts != 0 && (sals.nelts != 1 || sals_end.nelts != 1))
> +    {
> +      /* Ranged breakpoints have only one start location and one end
> +	 location.  */
> +      b->enable_state = bp_disabled;
> +      update_global_location_list (1);
> +      printf_unfiltered (_("Could not reset ranged breakpoint %d: "
> +			   "multiple locations found\n"),
> +			 b->number);
> +      return;
> +    }
> 

I don't understand why assert is replaced by a condition check.
Could you elaborate a little?

>    /* If there's no new locations, and all existing locations are
>       pending, don't do anything.  This optimizes the common case where
> @@ -11635,8 +11513,11 @@ update_breakpoint_locations (struct breakpoint *b,
>  
>    for (i = 0; i < sals.nelts; ++i)
>      {
> -      struct bp_location *new_loc = 
> -	add_location_to_breakpoint (b, &(sals.sals[i]));
> +      struct bp_location *new_loc;
> +
> +      switch_to_program_space_and_thread (sals.sals[i].pspace);
> +
> +      new_loc = add_location_to_breakpoint (b, &(sals.sals[i]));
>  
>        /* Reparse conditions, they might contain references to the
>  	 old symtab.  */
> @@ -11660,16 +11541,6 @@ update_breakpoint_locations (struct breakpoint *b,
>  	    }
>  	}
>  
> -      if (b->source_file != NULL)
> -	xfree (b->source_file);
> -      if (sals.sals[i].symtab == NULL)
> -	b->source_file = NULL;
> -      else
> -	b->source_file = xstrdup (sals.sals[i].symtab->filename);
> -
> -      if (b->line_number == 0)
> -	b->line_number = sals.sals[i].line;
> -
>        if (sals_end.nelts)
>  	{
>  	  CORE_ADDR end = find_breakpoint_range_end (sals_end.sals[0]);
> diff --git a/gdb/testsuite/gdb.linespec/linespec.exp b/gdb/testsuite/gdb.linespec/linespec.exp
> new file mode 100644
> index 0000000..122a468
> --- /dev/null
> +++ b/gdb/testsuite/gdb.linespec/linespec.exp
> @@ -0,0 +1,106 @@
> +# Copyright 2011 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Tests of ambiguous linespecs.
> +
> +set testfile linespec
> +
> +set exefile lspec
> +set binfile ${objdir}/${subdir}/${exefile}
> +
> +set baseone base/one/thefile.cc
> +set basetwo base/two/thefile.cc
> +
> +set hex {0x[0-9a-fA-F]+}
> +

${hex} has been defined in dejagnu/runtest.exp, and we can simply
use it.


-- 
Yao (齐尧)



More information about the Gdb-patches mailing list