From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11999 invoked by alias); 12 Nov 2011 02:40:10 -0000 Received: (qmail 11944 invoked by uid 22791); 12 Nov 2011 02:40:06 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,TW_TD,TW_TP 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; Sat, 12 Nov 2011 02:39:48 +0000 Received: from nat-jpt.mentorg.com ([192.94.33.2] helo=PR1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RP3VT-0001k8-Uk from Yao_Qi@mentor.com for gdb-patches@sourceware.org; Fri, 11 Nov 2011 18:39:48 -0800 Received: from [127.0.0.1] ([172.16.63.104]) by PR1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Sat, 12 Nov 2011 11:39:45 +0900 Message-ID: <4EBDDC6F.3070402@codesourcery.com> Date: Sat, 12 Nov 2011 02:40: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: Pedro Alves CC: gdb-patches@sourceware.org Subject: Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running References: <4EB8C551.9090609@codesourcery.com> <4EB8CEC4.9000905@codesourcery.com> <201111101653.13908.pedro@codesourcery.com> In-Reply-To: <201111101653.13908.pedro@codesourcery.com> Content-Type: multipart/mixed; boundary="------------070601050409010806070004" 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/msg00338.txt.bz2 This is a multi-part message in MIME format. --------------070601050409010806070004 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-length: 5885 On 11/11/2011 12:53 AM, Pedro Alves wrote: >> > @@ -1666,10 +1670,48 @@ add_tracepoint (int num, CORE_ADDR addr) >> > tpoint->handle = NULL; >> > tpoint->next = NULL; >> > >> > - if (!last_tracepoint) >> > - tracepoints = tpoint; >> > + if (sort) >> > + { >> > + struct tracepoint *tp, *tp_prev; >> > + >> > + /* Find a place to insert this tracepoint into list in order to keep >> > + the tracepoint list still in an ascending order. There may be >> > + multiple tracepoints at the same address as TPOINT's, and this >> > + guarantee that TP_PREV is the last tracepoint entry of them, so that >> > + TPOINT is inserted at the last of them. For example, fast tracepoint >> > + A, B, C are set at the same address, and D is to be insert at the same >> > + place as well, >> > + >> > + -->| A |--> | B |-->| C |->... >> > + >> > + One jump pad was created for tracepoint A, B, and C, and the target >> > + address of A is referenced/used in jump pad. So jump pad will let >> > + inferior jump to A. If D is inserted in front of A, like this, >> > + >> > + -->| D |-->| A |--> | B |-->| C |->... >> > + >> > + without updating jump pad, D is not reachable during collect, which >> > + is wrong. As we can see, the order of B, C and D doesn't matter, but >> > + A should always be the `first' one. */ >> > + for (tp_prev = tp = tracepoints; tp && tp->address <= tpoint->address; >> > + tp = tp->next) >> > + tp_prev = tp; >> > + >> > + if (tp_prev) >> > + { >> > + tpoint->next = tp_prev->next; >> > + tp_prev->next = tpoint; >> > + } >> > + else >> > + tracepoints = tpoint; > This looks incorrect. If there was only one tracepoint on the list, > and its address was higher than TPOINT's, then you'd end up with > > tp_prev = tracepoints; // the existing tracepoint > > and then this > > + if (tp_prev) > + { > + tpoint->next = tp_prev->next; > + tp_prev->next = tpoint; > + } > > inserts the new tracepoint after that existing > tracepoint, but it should be before. Hmmm, you are right. > > IOW, tp_prev should start out NULL: > > for (tp_prev = NULL, tp = tracepoints; > tp != NULL && tp->address <= tpoint->address; > tp_prev = tp, tp = tp->next) > ; > > if (tp_prev) > { > tpoint->next = tp_prev->next; > tp_prev->next = tpoint; > } > else > { > tpoint->next = tracepoints; > tracepoints = tpoint; > } > > which can be written in a more compact form as: > > struct tracepoint **tp_next; > > for (tp_next = &tracepoints; > (*tp_next) != NULL && (*tp_next)->address <= tpoint->address; > tp_next = &(*tp_next)->next) > ; > tpoint->next = *tp_next; > *tp_next = tpoint; > This compact form is great! Thanks. > >> > + } >> > else >> > - last_tracepoint->next = tpoint; >> > + { >> > + if (!last_tracepoint) >> > + tracepoints = tpoint; >> > + else >> > + last_tracepoint->next = tpoint; >> > + } >> > last_tracepoint = tpoint; >> > > If we have a path that needs a sorted insert, how about we > always do the inserted sort, and get rid of the non-inserted > sort, and the sort at tstart time? That'd mean less code to > maintain. > That is fine. sort_tracepoints is removed. > >> > seen_step_action_flag = 0; >> > @@ -2269,6 +2311,8 @@ static void >> > + >> > +/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */ >> > + >> > +static void >> > +install_tracepoint (struct tracepoint *tpoint, char *own_buf) >> > +{ >> > + tpoint->handle = NULL; >> > + >> > + if (tpoint->type == trap_tracepoint) >> > + { >> > + /* Tracepoints are installed as memory breakpoints. Just go >> > + ahead and install the trap. The breakpoints module >> > + handles duplicated breakpoints, and the memory read >> > + routine handles un-patching traps from memory reads. */ >> > + tpoint->handle = set_breakpoint_at (tpoint->address, >> > + tracepoint_handler); >> > + } >> > + else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint) >> > + { >> > + struct tracepoint *tp; >> > + >> > + if (!in_process_agent_loaded ()) >> > + { >> > + trace_debug ("Requested a %s tracepoint, but fast " >> > + "tracepoints aren't supported.", >> > + tpoint->type == static_tracepoint ? "static" : "fast"); >> > + write_e_ipa_not_loaded (own_buf); >> > + unpause_all (1); > This unpause_all is a leftover from a previous version. You now unpause > at the caller, so remove this. > Removed. >> > + return; >> > + } >> > + if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ()) >> > + { >> > + trace_debug ("Requested a static tracepoint, but static " >> > + "tracepoints are not supported."); >> > + write_e_ust_not_loaded (own_buf); >> > + unpause_all (1); > Ditto. > Removed. >> > + else >> > + internal_error (__FILE__, __LINE__, "Unknown tracepoint type"); >> > + >> > + if (tpoint->handle == NULL) >> > + { >> > + if (tpoint->type == fast_tracepoint) >> > + write_enn (own_buf); > See cmd_qtstart: > > *packet = '\0'; > ... > if (*packet == '\0') > write_enn (packet); > > We should use the same pattern: > > *own_buf = '\0'; > ... > if (*own_buf == '\0') > write_enn (own_buf); > > Because probe_marker_at only fills the error string > on a class of errors. OK, I understand now. -- Yao (齐尧) --------------070601050409010806070004 Content-Type: text/x-patch; name="0006-gdbserver-install-tp-if-in-tracing.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0006-gdbserver-install-tp-if-in-tracing.patch" Content-length: 10494 * server.c (handle_query): Handle InstallInTrace for qSupported. * tracepoint.c (add_tracepoint): Sort list. (install_tracepoint, download_tracepoint): New. (cmd_qtdp): Call them to install and download tracepoints. (sort_tracepoints): Removed. (cmd_qtstart): Update. --- gdb/gdbserver/server.c | 1 + gdb/gdbserver/tracepoint.c | 248 ++++++++++++++++++++++++++++++++----------- 2 files changed, 185 insertions(+), 64 deletions(-) diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c index 4612457..0f963e8 100644 --- a/gdb/gdbserver/server.c +++ b/gdb/gdbserver/server.c @@ -1584,6 +1584,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) if (gdb_supports_qRelocInsn && target_supports_fast_tracepoints ()) strcat (own_buf, ";FastTracepoints+"); strcat (own_buf, ";StaticTracepoints+"); + strcat (own_buf, ";InstallInTrace+"); strcat (own_buf, ";qXfer:statictrace:read+"); strcat (own_buf, ";qXfer:traceframe-info:read+"); strcat (own_buf, ";EnableDisableTracepoints+"); diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c index 3a6a0f3..f000f63 100644 --- a/gdb/gdbserver/tracepoint.c +++ b/gdb/gdbserver/tracepoint.c @@ -1245,6 +1245,9 @@ static void do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx, #ifndef IN_PROCESS_AGENT static struct tracepoint *fast_tracepoint_from_ipa_tpoint_address (CORE_ADDR); + +static void install_tracepoint (struct tracepoint *, char *own_buf); +static void download_tracepoint (struct tracepoint *); static int install_fast_tracepoint (struct tracepoint *); #endif @@ -1641,12 +1644,13 @@ free_space (void) static int seen_step_action_flag; -/* Create a tracepoint (location) with given number and address. */ +/* Create a tracepoint (location) with given number and address. Add this + new tracepoint to list and sort this list. */ static struct tracepoint * add_tracepoint (int num, CORE_ADDR addr) { - struct tracepoint *tpoint; + struct tracepoint *tpoint, **tp_next; tpoint = xmalloc (sizeof (struct tracepoint)); tpoint->number = num; @@ -1666,10 +1670,31 @@ add_tracepoint (int num, CORE_ADDR addr) tpoint->handle = NULL; tpoint->next = NULL; - if (!last_tracepoint) - tracepoints = tpoint; - else - last_tracepoint->next = tpoint; + /* Find a place to insert this tracepoint into list in order to keep + the tracepoint list still in the ascending order. There may be + multiple tracepoints at the same address as TPOINT's, and this + guarantees TPOINT is inserted after all the tracepoints which are + set at the same address. For example, fast tracepoints A, B, C are + set at the same address, and D is to be insert at the same place as + well, + + -->| A |--> | B |-->| C |->... + + One jump pad was created for tracepoint A, B, and C, and the target + address of A is referenced/used in jump pad. So jump pad will let + inferior jump to A. If D is inserted in front of A, like this, + + -->| D |-->| A |--> | B |-->| C |->... + + without updating jump pad, D is not reachable during collect, which + is wrong. As we can see, the order of B, C and D doesn't matter, but + A should always be the `first' one. */ + for (tp_next = &tracepoints; + (*tp_next) != NULL && (*tp_next)->address <= tpoint->address; + tp_next = &(*tp_next)->next) + ; + tpoint->next = *tp_next; + *tp_next = tpoint; last_tracepoint = tpoint; seen_step_action_flag = 0; @@ -2269,6 +2294,8 @@ static void cmd_qtdp (char *own_buf) { int tppacket; + /* Whether there is a trailing hyphen at the end of the QTDP packet. */ + int trail_hyphen = 0; ULONGEST num; ULONGEST addr; ULONGEST count; @@ -2346,7 +2373,10 @@ cmd_qtdp (char *own_buf) trace_debug ("Unknown optional tracepoint field"); } if (*packet == '-') - trace_debug ("Also has actions\n"); + { + trail_hyphen = 1; + trace_debug ("Also has actions\n"); + } trace_debug ("Defined %stracepoint %d at 0x%s, " "enabled %d step %ld pass %ld", @@ -2365,6 +2395,29 @@ cmd_qtdp (char *own_buf) return; } + /* Install tracepoint during tracing only once for each tracepoint location. + For each tracepoint loc, GDB may send multiple QTDP packets, and we can + determine the last QTDP packet for one tracepoint location by checking + trailing hyphen in QTDP packet. */ + if (tracing && !trail_hyphen) + { + /* Pause all threads temporarily while we patch tracepoints. */ + pause_all (0); + + /* download_tracepoint will update global `tracepoints' + list, so it is unsafe to leave threads in jump pad. */ + stabilize_threads (); + + /* Freeze threads. */ + pause_all (1); + + download_tracepoint (tpoint); + install_tracepoint (tpoint, own_buf); + + unpause_all (1); + return; + } + write_ok (own_buf); } @@ -2658,59 +2711,6 @@ claim_jump_space (ULONGEST used) gdb_jump_pad_head += used; } -/* Sort tracepoints by PC, using a bubble sort. */ - -static void -sort_tracepoints (void) -{ - struct tracepoint *lst, *tmp, *prev = NULL; - int i, j, n = 0; - - if (tracepoints == NULL) - return; - - /* Count nodes. */ - for (tmp = tracepoints; tmp->next; tmp = tmp->next) - n++; - - for (i = 0; i < n - 1; i++) - for (j = 0, lst = tracepoints; - lst && lst->next && (j <= n - 1 - i); - j++) - { - /* If we're at beginning, the start node is the prev - node. */ - if (j == 0) - prev = lst; - - /* Compare neighbors. */ - if (lst->next->address < lst->address) - { - struct tracepoint *p; - - /* Swap'em. */ - tmp = (lst->next ? lst->next->next : NULL); - - if (j == 0 && prev == tracepoints) - tracepoints = lst->next; - - p = lst->next; - prev->next = lst->next; - lst->next->next = lst; - lst->next = tmp; - prev = p; - } - else - { - lst = lst->next; - /* Keep track of the previous node. We need it if we need - to swap nodes. */ - if (j != 0) - prev = prev->next; - } - } -} - /* Ask the IPA to probe the marker at ADDRESS. Returns -1 if running the command fails, or 0 otherwise. If the command ran successfully, but probing the marker failed, ERROUT will be filled @@ -2798,6 +2798,83 @@ install_fast_tracepoint (struct tracepoint *tpoint) return 0; } + +/* Install tracepoint TPOINT, and write reply message in OWN_BUF. */ + +static void +install_tracepoint (struct tracepoint *tpoint, char *own_buf) +{ + tpoint->handle = NULL; + *own_buf = '\0'; + + if (tpoint->type == trap_tracepoint) + { + /* Tracepoints are installed as memory breakpoints. Just go + ahead and install the trap. The breakpoints module + handles duplicated breakpoints, and the memory read + routine handles un-patching traps from memory reads. */ + tpoint->handle = set_breakpoint_at (tpoint->address, + tracepoint_handler); + } + else if (tpoint->type == fast_tracepoint || tpoint->type == static_tracepoint) + { + struct tracepoint *tp; + + if (!in_process_agent_loaded ()) + { + trace_debug ("Requested a %s tracepoint, but fast " + "tracepoints aren't supported.", + tpoint->type == static_tracepoint ? "static" : "fast"); + write_e_ipa_not_loaded (own_buf); + return; + } + if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ()) + { + trace_debug ("Requested a static tracepoint, but static " + "tracepoints are not supported."); + write_e_ust_not_loaded (own_buf); + return; + } + + /* Find another fast or static tracepoint at the same address. */ + for (tp = tracepoints; tp; tp = tp->next) + { + if (tp->address == tpoint->address && tp->type == tpoint->type + && tp->number != tpoint->number) + break; + } + + if (tpoint->type == fast_tracepoint) + { + if (tp) /* TPOINT is installed at the same address as TP. */ + clone_fast_tracepoint (tpoint, tp); + else + install_fast_tracepoint (tpoint); + } + else + { + if (tp) + tpoint->handle = (void *) -1; + else + { + if (probe_marker_at (tpoint->address, own_buf) == 0) + tpoint->handle = (void *) -1; + } + } + + } + else + internal_error (__FILE__, __LINE__, "Unknown tracepoint type"); + + if (tpoint->handle == NULL) + { + if (*own_buf == '\0') + write_enn (own_buf); + } + else + write_ok (own_buf); +} + static void cmd_qtstart (char *packet) { @@ -2805,10 +2882,6 @@ cmd_qtstart (char *packet) trace_debug ("Starting the trace"); - /* Sort tracepoints by ascending address. This makes installing - fast tracepoints at the same address easier to handle. */ - sort_tracepoints (); - /* Pause all threads temporarily while we patch tracepoints. */ pause_all (0); @@ -6462,6 +6535,53 @@ download_tracepoint_1 (struct tracepoint *tpoint) } static void +download_tracepoint (struct tracepoint *tpoint) +{ + struct tracepoint *tp, *tp_prev; + + if (tpoint->type != fast_tracepoint + && tpoint->type != static_tracepoint) + return; + + download_tracepoint_1 (tpoint); + + /* Find the previous entry of TPOINT, which is fast tracepoint or + static tracepoint. */ + tp_prev = NULL; + for (tp = tracepoints; tp != tpoint; tp = tp->next) + { + if (tp->type == fast_tracepoint || tp->type == static_tracepoint) + tp_prev = tp; + } + + if (tp_prev) + { + CORE_ADDR tp_prev_target_next_addr; + + /* Insert TPOINT after TP_PREV in IPA. */ + if (read_inferior_data_pointer (tp_prev->obj_addr_on_target + + offsetof (struct tracepoint, next), + &tp_prev_target_next_addr)) + fatal ("error reading `tp_prev->next'"); + + /* tpoint->next = tp_prev->next */ + write_inferior_data_ptr (tpoint->obj_addr_on_target + + offsetof (struct tracepoint, next), + tp_prev_target_next_addr); + /* tp_prev->next = tpoint */ + write_inferior_data_ptr (tp_prev->obj_addr_on_target + + offsetof (struct tracepoint, next), + tpoint->obj_addr_on_target); + } + else + /* First object in list, set the head pointer in the + inferior. */ + write_inferior_data_ptr (ipa_sym_addrs.addr_tracepoints, + tpoint->obj_addr_on_target); + +} + +static void download_tracepoints (void) { CORE_ADDR tpptr = 0, prev_tpptr = 0; -- 1.7.0.4 --------------070601050409010806070004--