Re: [patch 4/8] Download tracepoint locations and track its status

Pedro Alves
2011-11-10T15:51:38Z

On Tuesday 08 November 2011 06:30:17, Yao Qi wrote:

> @@ -10660,6 +10713,8 @@ update_global_location_list (int should_insert)
> || (gdbarch_has_global_breakpoints (target_gdbarch))))
> insert_breakpoint_locations ();
>
> + download_tracepoint_locations ();

Should be guarded by `should_insert'.

> - /* Skip LOCP entries which will definitely never be needed.
> + /* skip LOCP entries which will definitely never be needed.

Drop this spurious change please.

> @@ -10491,7 +10539,9 @@ update_global_location_list (int should_insert)
> if it should be inserted in case it will be
> unduplicated. */
> if (loc2 != old_loc
> - && unduplicated_should_be_inserted (loc2))
> + && unduplicated_should_be_inserted (loc2)
> + && (is_tracepoint (old_loc->owner)
> + == is_tracepoint (loc2->owner)))

What if both are tracepoints, but of different kind? I think this
might be better handled within breakpoint_locations_match (called just above).

> /* Nonzero if this is not the first breakpoint in the list
> - for the given address. */
> + for the given address. In current implementation, location
> + of tracepoint never be duplicated with other locations of
> + tracepoints and other kinds of breakpoints, because two locations
> + at the same address may have different actions, so both of
> + these locations should be downloaded.

and so that "tfind N" always works.

Please drop the "In current implementation" bit, and the "it is
possible" comment.

> + /* Locations of tracepoints never be duplicated. */

A verb is missing. "can never be", perhaps?

> + if (is_tracepoint (left->owner))
> + gdb_assert (left->duplicate == 0);

`duplicate' is a boolean. Make that `gdb_assert (!left->duplicate)'.

> + if (is_tracepoint (right->owner))
> + gdb_assert (right->duplicate == 0);

> + gdb_assert (loc->inserted == 0);

Boolean ==> `gdb_assert (!loc->inserted)'

--
Pedro Alves