From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18448 invoked by alias); 16 Nov 2011 08:15:54 -0000 Received: (qmail 18428 invoked by uid 22791); 16 Nov 2011 08:15:48 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 16 Nov 2011 08:15:31 +0000 Received: from nat-jpt.mentorg.com ([192.94.33.2] helo=PR1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RQaeY-0007XV-J9 from Yao_Qi@mentor.com ; Wed, 16 Nov 2011 00:15:30 -0800 Received: from [127.0.0.1] ([172.16.63.104]) by PR1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Wed, 16 Nov 2011 17:15:28 +0900 Message-ID: <4EC3711B.5070206@codesourcery.com> Date: Wed, 16 Nov 2011 08:15:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:7.0.1) Gecko/20110930 Thunderbird/7.0.1 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: RFA: implement ambiguous linespec proposal References: <20111028221459.GA28467@host1.jankratochvil.net> <20111104074543.GA13839@host1.jankratochvil.net> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-11/txt/msg00421.txt.bz2 On 11/15/2011 05:10 AM, Tom Tromey wrote: >>>>>> "Tom" == Tom Tromey 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 = ©_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 . > + > +# 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 (齐尧)