Re: RFA: implement ambiguous linespec proposal

Yao Qi
2011-11-16T08:15:54Z

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 (é½å°§)