From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13867 invoked by alias); 10 Nov 2011 16:53:40 -0000 Received: (qmail 13856 invoked by uid 22791); 10 Nov 2011 16:53:38 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,TW_TD 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; Thu, 10 Nov 2011 16:53:21 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=EU1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ROXsO-0003V6-UQ from pedro_alves@mentor.com for gdb-patches@sourceware.org; Thu, 10 Nov 2011 08:53:21 -0800 Received: from scottsdale.localnet ([172.16.63.104]) by EU1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Thu, 10 Nov 2011 16:53:17 +0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch 6/8] gdbserver - Install tracepoint when tracing is running Date: Thu, 10 Nov 2011 16:53:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-12-generic; KDE/4.7.2; x86_64; ; ) Cc: Yao Qi References: <4EB8C551.9090609@codesourcery.com> <4EB8CEC4.9000905@codesourcery.com> In-Reply-To: <4EB8CEC4.9000905@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201111101653.13908.pedro@codesourcery.com> 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/msg00279.txt.bz2 On Tuesday 08 November 2011 06:40:04, Yao Qi 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. 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; > + } > 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. > seen_step_action_flag = 0; > @@ -2269,6 +2311,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; > @@ -2306,7 +2350,11 @@ cmd_qtdp (char *own_buf) > return; > } > > - tpoint = add_tracepoint (num, addr); > + /* When it is not tracing, we don't have to sort tracepoints, because > + they will be sorted in cmd_qtstart later. When it is tracing, the > + list has been sorted, and we should still keep ascending order of > + list after adding new entry. */ > + tpoint = add_tracepoint (num, addr, tracing); > > tpoint->enabled = (*packet == 'E'); > ++packet; /* skip 'E' */ > @@ -2346,7 +2394,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 +2416,29 @@ cmd_qtdp (char *own_buf) > return; > } > > + /* Install tracepoint during tracing only once of 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); > } > > @@ -2798,6 +2872,84 @@ 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; > + > + 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. > + 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. > + 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 (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. -- Pedro Alves