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

Yao Qi
2011-11-12T02:11:28Z

[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]

On 11/10/2011 11:51 PM, Pedro Alves wrote:
> 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'.
>

Right. Fixed.

>> - /* Skip LOCP entries which will definitely never be needed.
>> + /* skip LOCP entries which will definitely never be needed.
>
> Drop this spurious change please.

Removed.
>
>> @@ -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).
>

I move this part into a new function tracepoint_locations_match, and
call it in breakpoint_locations_match.

>
>> /* 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.

OK, comment is changed as you suggested.

>
>> + /* Locations of tracepoints never be duplicated. */
>
> A verb is missing. "can never be", perhaps?
>

Oops. Fixed.

>> + 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)'
>

Fixed.

--
Yao (é½å°§)

[-- Attachment #2: 0004-tracepoint-change-loc.patch --]
[-- Type: text/x-patch, Size: 10124 bytes --]

* breakpoint.h (struct bp_location): Add comment on field
`duplicate'.
* breakpoint.c (should_be_inserted): Don't differentiate breakpoint
and tracepoint.
(remove_breakpoints): Don't remove tracepoints.
(tracepoint_locations_match ): New.
(breakpoint_locations_match): Call it.
(disable_breakpoints_in_unloaded_shlib): Handle tracepoint.
(download_tracepoint_locations): New.
(update_global_location_list): Call it.
* tracepoint.c (find_matching_tracepoint): Delete.
(find_matching_tracepoint_location): Renamed from
find_matching_tracepoint. Return bp_location rather than
tracepoint.
(merge_uploaded_tracepoints): Set `inserted' field to 1 if
tracepoint is found.
---
gdb/breakpoint.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++--------
gdb/breakpoint.h | 6 +++-
gdb/tracepoint.c | 39 ++++++++++++++------
3 files changed, 120 insertions(+), 28 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 8c98bef..55d707d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1555,7 +1555,10 @@ in which its expression is valid.\n"),

/* Returns 1 iff breakpoint location should be
- inserted in the inferior. */
+ inserted in the inferior. We don't differentiate the type of BL's owner
+ (breakpoint vs. tracepoint), although insert_location in tracepoint's
+ breakpoint_ops is not defined, because in insert_bp_location,
+ tracepoint's insert_location will not be called. */
static int
should_be_inserted (struct bp_location *bl)
{
@@ -1579,11 +1582,6 @@ should_be_inserted (struct bp_location *bl)
if (bl->pspace->breakpoints_not_allowed)
return 0;

- /* Tracepoints are inserted by the target at a time of its choosing,
- not by us. */
- if (is_tracepoint (bl->owner))
- return 0;
-
return 1;
}

@@ -2049,7 +2047,7 @@ remove_breakpoints (void)

ALL_BP_LOCATIONS (bl, blp_tmp)
{
- if (bl->inserted)
+ if (bl->inserted && !is_tracepoint (bl->owner))
val |= remove_breakpoint (bl, mark_uninserted);
}
return val;
@@ -5443,6 +5441,23 @@ breakpoint_location_address_match (struct bp_location *bl,
aspace, addr)));
}

+/* If LOC1 and LOC2's owners are not tracepoints, returns false directly.
+ Then, if LOC1 and LOC2 represent the same tracepoint location, returns
+ true, otherwise returns false. */
+
+static int
+tracepoint_locations_match (struct bp_location *loc1,
+ struct bp_location *loc2)
+{
+ if (is_tracepoint (loc1->owner) && is_tracepoint (loc2->owner))
+ /* Since tracepoint locations are never duplicated with others', tracepoint
+ locations at the same address of different tracepoints are regarded as
+ different locations. */
+ return (loc1->address == loc2->address && loc1->owner == loc2->owner);
+ else
+ return 0;
+}
+
/* Assuming LOC1 and LOC2's types' have meaningful target addresses
(breakpoint_address_is_meaningful), returns true if LOC1 and LOC2
represent the same location. */
@@ -5464,6 +5479,8 @@ breakpoint_locations_match (struct bp_location *loc1,
return 0;
else if (hw_point1)
return watchpoint_locations_match (loc1, loc2);
+ else if (is_tracepoint (loc1->owner) || is_tracepoint (loc2->owner))
+ return tracepoint_locations_match (loc1, loc2);
else
/* We compare bp_location.length in order to cover ranged breakpoints. */
return (breakpoint_address_match (loc1->pspace->aspace, loc1->address,
@@ -6071,8 +6088,8 @@ disable_breakpoints_in_shlibs (void)
}
}

-/* Disable any breakpoints that are in an unloaded shared library.
- Only apply to enabled breakpoints, disabled ones can just stay
+/* Disable any breakpoints and tracepoints that are in an unloaded shared
+ library. Only apply to enabled breakpoints, disabled ones can just stay
disabled. */

static void
@@ -6094,13 +6111,14 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
/* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL. */
struct breakpoint *b = loc->owner;

- if ((loc->loc_type == bp_loc_hardware_breakpoint
- || loc->loc_type == bp_loc_software_breakpoint)
- && solib->pspace == loc->pspace
+ if (solib->pspace == loc->pspace
&& !loc->shlib_disabled
- && (b->type == bp_breakpoint
- || b->type == bp_jit_event
- || b->type == bp_hardware_breakpoint)
+ && (((b->type == bp_breakpoint
+ || b->type == bp_jit_event
+ || b->type == bp_hardware_breakpoint)
+ && (loc->loc_type == bp_loc_hardware_breakpoint
+ || loc->loc_type == bp_loc_software_breakpoint))
+ || is_tracepoint (b))
&& solib_contains_address_p (solib, loc->address))
{
loc->shlib_disabled = 1;
@@ -10325,6 +10343,49 @@ bp_location_target_extensions_update (void)
}
}

+/* Download tracepoint locations if they haven't been. */
+
+static void
+download_tracepoint_locations (void)
+{
+ struct bp_location *bl, **blp_tmp;
+ struct cleanup *old_chain;
+
+ if (!target_can_download_tracepoint ())
+ return;
+
+ old_chain = save_current_space_and_thread ();
+
+ ALL_BP_LOCATIONS (bl, blp_tmp)
+ {
+ struct tracepoint *t;
+
+ if (!is_tracepoint (bl->owner))
+ continue;
+
+ if ((bl->owner->type == bp_fast_tracepoint
+ ? !may_insert_fast_tracepoints
+ : !may_insert_tracepoints))
+ continue;
+
+ /* In tracepoint, locations are _never_ duplicated, so
+ should_be_inserted is equivalent to
+ unduplicated_should_be_inserted. */
+ if (!should_be_inserted (bl) || bl->inserted)
+ continue;
+
+ switch_to_program_space_and_thread (bl->pspace);
+
+ target_download_tracepoint (bl);
+
+ bl->inserted = 1;
+ t = (struct tracepoint *) bl->owner;
+ t->number_on_target = bl->owner->number;
+ }
+
+ do_cleanups (old_chain);
+}
+
/* Swap the insertion/duplication state between two locations. */

static void
@@ -10334,6 +10395,12 @@ swap_insertion (struct bp_location *left, struct bp_location *right)
const int left_duplicate = left->duplicate;
const struct bp_target_info left_target_info = left->target_info;

+ /* Locations of tracepoints can never be duplicated. */
+ if (is_tracepoint (left->owner))
+ gdb_assert (!left->duplicate);
+ if (is_tracepoint (right->owner))
+ gdb_assert (!right->duplicate);
+
left->inserted = right->inserted;
left->duplicate = right->duplicate;
left->target_info = right->target_info;
@@ -10613,6 +10680,9 @@ update_global_location_list (int should_insert)
|| !loc->enabled
|| loc->shlib_disabled
|| !breakpoint_address_is_meaningful (b)
+ /* Don't detect duplicate for tracepoint locations because they are
+ never duplicated. See the comments in field `duplicate' of
+ `struct bp_location'. */
|| is_tracepoint (b))
continue;

@@ -10660,6 +10730,9 @@ update_global_location_list (int should_insert)
|| (gdbarch_has_global_breakpoints (target_gdbarch))))
insert_breakpoint_locations ();

+ if (should_insert)
+ download_tracepoint_locations ();
+
do_cleanups (cleanups);
}

diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index fe381df..506ae80 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -335,7 +335,11 @@ struct bp_location
char inserted;

/* Nonzero if this is not the first breakpoint in the list
- for the given address. */
+ for the given address. location of tracepoint can _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. */
char duplicate;

/* If we someday support real thread-specific breakpoints, then
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index a7035d1..82ca0b8 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1711,7 +1711,15 @@ start_tracing (void)
t->number_on_target = 0;

for (loc = b->loc; loc; loc = loc->next)
- target_download_tracepoint (loc);
+ {
+ /* Since tracepoint locations are never duplicated, `inserted'
+ flag should be zero. */
+ gdb_assert (!loc->inserted);
+
+ target_download_tracepoint (loc);
+
+ loc->inserted = 1;
+ }

t->number_on_target = b->number;
}
@@ -3203,10 +3211,10 @@ cond_string_is_same (char *str1, char *str2)
/* Look for an existing tracepoint that seems similar enough to the
uploaded one. Enablement isn't compared, because the user can
toggle that freely, and may have done so in anticipation of the
- next trace run. */
+ next trace run. Return the location of matched tracepoint. */

-struct tracepoint *
-find_matching_tracepoint (struct uploaded_tp *utp)
+struct bp_location *
+find_matching_tracepoint_location (struct uploaded_tp *utp)
{
VEC(breakpoint_p) *tp_vec = all_tracepoints ();
int ix;
@@ -3228,7 +3236,7 @@ find_matching_tracepoint (struct uploaded_tp *utp)
for (loc = b->loc; loc; loc = loc->next)
{
if (loc->address == utp->addr)
- return t;
+ return loc;
}
}
}
@@ -3243,17 +3251,24 @@ void
merge_uploaded_tracepoints (struct uploaded_tp **uploaded_tps)
{
struct uploaded_tp *utp;
- struct tracepoint *t;

/* Look for GDB tracepoints that match up with our uploaded versions. */
for (utp = *uploaded_tps; utp; utp = utp->next)
{
- t = find_matching_tracepoint (utp);
- if (t)
- printf_filtered (_("Assuming tracepoint %d is same "
- "as target's tracepoint %d at %s.\n"),
- t->base.number, utp->number,
- paddress (get_current_arch (), utp->addr));
+ struct bp_location *loc;
+ struct tracepoint *t;
+
+ loc = find_matching_tracepoint_location (utp);
+ if (loc)
+ {
+ /* Mark this location as already inserted. */
+ loc->inserted = 1;
+ t = (struct tracepoint *) loc->owner;
+ printf_filtered (_("Assuming tracepoint %d is same "
+ "as target's tracepoint %d at %s.\n"),
+ loc->owner->number, utp->number,
+ paddress (loc->gdbarch, utp->addr));
+ }
else
{
t = create_tracepoint_from_upload (utp);
--
1.7.0.4