From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12170 invoked by alias); 9 Nov 2011 03:34:58 -0000 Received: (qmail 11361 invoked by uid 22791); 9 Nov 2011 03:34:55 -0000 X-SWARE-Spam-Status: No, hits=-1.6 required=5.0 tests=AWL,BAYES_00,TW_CP 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; Wed, 09 Nov 2011 03:34:38 +0000 Received: from nat-jpt.mentorg.com ([192.94.33.2] helo=PR1-MAIL.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1RNyvt-0001Fy-4x from Yao_Qi@mentor.com for gdb-patches@sourceware.org; Tue, 08 Nov 2011 19:34:37 -0800 Received: from [127.0.0.1] ([172.16.63.104]) by PR1-MAIL.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.1830); Wed, 9 Nov 2011 12:34:21 +0900 Message-ID: <4EB9F4A9.90602@codesourcery.com> Date: Wed, 09 Nov 2011 03:34: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: gdb-patches@sourceware.org Subject: Re: [patch 1/8] Download tracepoint on location level References: <4EB8C551.9090609@codesourcery.com> <4EB8C74B.8050702@codesourcery.com> In-Reply-To: <4EB8C74B.8050702@codesourcery.com> Content-Type: multipart/mixed; boundary="------------070003050107000002030707" 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/msg00218.txt.bz2 This is a multi-part message in MIME format. --------------070003050107000002030707 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-length: 477 On 11/08/2011 02:08 PM, Yao Qi wrote: > This patch is to refactor to_download_tracepoint to > to_download_tracepoint_loc. Beside this refactor, a return value is > added in new hook to reflect whether tracepoint location is > downloaded/installed successfully. Functionality of gdb is not changed. I re-read this patch again, and find the return value is unnecessary, because remote_download_tracepoint_loc executes successfully if no error is thrown out. -- Yao (齐尧) --------------070003050107000002030707 Content-Type: text/x-patch; name="0001-refactor-download_tracepoint-to-download_tracepoint_.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0001-refactor-download_tracepoint-to-download_tracepoint_.pa"; filename*1="tch" Content-length: 15560 * target.h (struct target): Rename field to_download_tracepoint to to_download_tracepoint_loc. Change type of parameter from tracepoint bp_location. * target.c (update_current_target): Update. * tracepoint.c (start_tracing): Update. * remote.c: Move remote_download_tracepoint. to remote_download_tracepoint_loc. Remove loop for each location of a tracepoint. --- gdb/remote.c | 279 ++++++++++++++++++++++++++--------------------------- gdb/target.c | 6 +- gdb/target.h | 10 +- gdb/tracepoint.c | 6 +- 4 files changed, 151 insertions(+), 150 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 5182ef1..5f7a466 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -9835,9 +9835,9 @@ remote_download_command_source (int num, ULONGEST addr, } static void -remote_download_tracepoint (struct breakpoint *b) +remote_download_tracepoint_loc (struct bp_location *loc) { - struct bp_location *loc; + CORE_ADDR tpaddr; char addrbuf[40]; char buf[2048]; @@ -9848,169 +9848,164 @@ remote_download_tracepoint (struct breakpoint *b) struct agent_expr *aexpr; struct cleanup *aexpr_chain = NULL; char *pkt; + struct breakpoint *b = loc->owner; struct tracepoint *t = (struct tracepoint *) b; - /* Iterate over all the tracepoint locations. It's up to the target to - notice multiple tracepoint packets with the same number but different - addresses, and treat them as multiple locations. */ - for (loc = b->loc; loc; loc = loc->next) - { - encode_actions (b, loc, &tdp_actions, &stepping_actions); - old_chain = make_cleanup (free_actions_list_cleanup_wrapper, - tdp_actions); - (void) make_cleanup (free_actions_list_cleanup_wrapper, - stepping_actions); - - tpaddr = loc->address; - sprintf_vma (addrbuf, tpaddr); - sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number, - addrbuf, /* address */ - (b->enable_state == bp_enabled ? 'E' : 'D'), - t->step_count, t->pass_count); - /* Fast tracepoints are mostly handled by the target, but we can - tell the target how big of an instruction block should be moved - around. */ - if (b->type == bp_fast_tracepoint) + encode_actions (loc->owner, loc, &tdp_actions, &stepping_actions); + old_chain = make_cleanup (free_actions_list_cleanup_wrapper, + tdp_actions); + (void) make_cleanup (free_actions_list_cleanup_wrapper, + stepping_actions); + + tpaddr = loc->address; + sprintf_vma (addrbuf, tpaddr); + sprintf (buf, "QTDP:%x:%s:%c:%lx:%x", b->number, + addrbuf, /* address */ + (b->enable_state == bp_enabled ? 'E' : 'D'), + t->step_count, t->pass_count); + /* Fast tracepoints are mostly handled by the target, but we can + tell the target how big of an instruction block should be moved + around. */ + if (b->type == bp_fast_tracepoint) + { + /* Only test for support at download time; we may not know + target capabilities at definition time. */ + if (remote_supports_fast_tracepoints ()) { - /* Only test for support at download time; we may not know - target capabilities at definition time. */ - if (remote_supports_fast_tracepoints ()) - { - int isize; + int isize; - if (gdbarch_fast_tracepoint_valid_at (target_gdbarch, - tpaddr, &isize, NULL)) - sprintf (buf + strlen (buf), ":F%x", isize); - else - /* If it passed validation at definition but fails now, - something is very wrong. */ - internal_error (__FILE__, __LINE__, - _("Fast tracepoint not " - "valid during download")); - } + if (gdbarch_fast_tracepoint_valid_at (target_gdbarch, + tpaddr, &isize, NULL)) + sprintf (buf + strlen (buf), ":F%x", isize); else - /* Fast tracepoints are functionally identical to regular - tracepoints, so don't take lack of support as a reason to - give up on the trace run. */ - warning (_("Target does not support fast tracepoints, " - "downloading %d as regular tracepoint"), b->number); + /* If it passed validation at definition but fails now, + something is very wrong. */ + internal_error (__FILE__, __LINE__, + _("Fast tracepoint not " + "valid during download")); } - else if (b->type == bp_static_tracepoint) + else + /* Fast tracepoints are functionally identical to regular + tracepoints, so don't take lack of support as a reason to + give up on the trace run. */ + warning (_("Target does not support fast tracepoints, " + "downloading %d as regular tracepoint"), b->number); + } + else if (b->type == bp_static_tracepoint) + { + /* Only test for support at download time; we may not know + target capabilities at definition time. */ + if (remote_supports_static_tracepoints ()) { - /* Only test for support at download time; we may not know - target capabilities at definition time. */ - if (remote_supports_static_tracepoints ()) - { - struct static_tracepoint_marker marker; + struct static_tracepoint_marker marker; - if (target_static_tracepoint_marker_at (tpaddr, &marker)) - strcat (buf, ":S"); - else - error (_("Static tracepoint not valid during download")); - } + if (target_static_tracepoint_marker_at (tpaddr, &marker)) + strcat (buf, ":S"); else - /* Fast tracepoints are functionally identical to regular - tracepoints, so don't take lack of support as a reason - to give up on the trace run. */ - error (_("Target does not support static tracepoints")); + error (_("Static tracepoint not valid during download")); } - /* If the tracepoint has a conditional, make it into an agent - expression and append to the definition. */ - if (loc->cond) + else + /* Fast tracepoints are functionally identical to regular + tracepoints, so don't take lack of support as a reason + to give up on the trace run. */ + error (_("Target does not support static tracepoints")); + } + /* If the tracepoint has a conditional, make it into an agent + expression and append to the definition. */ + if (loc->cond) + { + /* Only test support at download time, we may not know target + capabilities at definition time. */ + if (remote_supports_cond_tracepoints ()) { - /* Only test support at download time, we may not know target - capabilities at definition time. */ - if (remote_supports_cond_tracepoints ()) - { - aexpr = gen_eval_for_expr (tpaddr, loc->cond); - aexpr_chain = make_cleanup_free_agent_expr (aexpr); - sprintf (buf + strlen (buf), ":X%x,", aexpr->len); - pkt = buf + strlen (buf); - for (ndx = 0; ndx < aexpr->len; ++ndx) - pkt = pack_hex_byte (pkt, aexpr->buf[ndx]); - *pkt = '\0'; - do_cleanups (aexpr_chain); - } - else - warning (_("Target does not support conditional tracepoints, " - "ignoring tp %d cond"), b->number); + aexpr = gen_eval_for_expr (tpaddr, loc->cond); + aexpr_chain = make_cleanup_free_agent_expr (aexpr); + sprintf (buf + strlen (buf), ":X%x,", aexpr->len); + pkt = buf + strlen (buf); + for (ndx = 0; ndx < aexpr->len; ++ndx) + pkt = pack_hex_byte (pkt, aexpr->buf[ndx]); + *pkt = '\0'; + do_cleanups (aexpr_chain); } + else + warning (_("Target does not support conditional tracepoints, " + "ignoring tp %d cond"), b->number); + } if (b->commands || *default_collect) - strcat (buf, "-"); - putpkt (buf); - remote_get_noisy_reply (&target_buf, &target_buf_size); - if (strcmp (target_buf, "OK")) - error (_("Target does not support tracepoints.")); + strcat (buf, "-"); + putpkt (buf); + remote_get_noisy_reply (&target_buf, &target_buf_size); + if (strcmp (target_buf, "OK")) + error (_("Target does not support tracepoints.")); - /* do_single_steps (t); */ - if (tdp_actions) + /* do_single_steps (t); */ + if (tdp_actions) + { + for (ndx = 0; tdp_actions[ndx]; ndx++) { - for (ndx = 0; tdp_actions[ndx]; ndx++) - { - QUIT; /* Allow user to bail out with ^C. */ - sprintf (buf, "QTDP:-%x:%s:%s%c", - b->number, addrbuf, /* address */ - tdp_actions[ndx], - ((tdp_actions[ndx + 1] || stepping_actions) - ? '-' : 0)); - putpkt (buf); - remote_get_noisy_reply (&target_buf, - &target_buf_size); - if (strcmp (target_buf, "OK")) - error (_("Error on target while setting tracepoints.")); - } + QUIT; /* Allow user to bail out with ^C. */ + sprintf (buf, "QTDP:-%x:%s:%s%c", + b->number, addrbuf, /* address */ + tdp_actions[ndx], + ((tdp_actions[ndx + 1] || stepping_actions) + ? '-' : 0)); + putpkt (buf); + remote_get_noisy_reply (&target_buf, + &target_buf_size); + if (strcmp (target_buf, "OK")) + error (_("Error on target while setting tracepoints.")); } - if (stepping_actions) + } + if (stepping_actions) + { + for (ndx = 0; stepping_actions[ndx]; ndx++) { - for (ndx = 0; stepping_actions[ndx]; ndx++) - { - QUIT; /* Allow user to bail out with ^C. */ - sprintf (buf, "QTDP:-%x:%s:%s%s%s", - b->number, addrbuf, /* address */ - ((ndx == 0) ? "S" : ""), - stepping_actions[ndx], - (stepping_actions[ndx + 1] ? "-" : "")); - putpkt (buf); - remote_get_noisy_reply (&target_buf, - &target_buf_size); - if (strcmp (target_buf, "OK")) - error (_("Error on target while setting tracepoints.")); - } + QUIT; /* Allow user to bail out with ^C. */ + sprintf (buf, "QTDP:-%x:%s:%s%s%s", + b->number, addrbuf, /* address */ + ((ndx == 0) ? "S" : ""), + stepping_actions[ndx], + (stepping_actions[ndx + 1] ? "-" : "")); + putpkt (buf); + remote_get_noisy_reply (&target_buf, + &target_buf_size); + if (strcmp (target_buf, "OK")) + error (_("Error on target while setting tracepoints.")); } + } - if (remote_protocol_packets[PACKET_TracepointSource].support - == PACKET_ENABLE) + if (remote_protocol_packets[PACKET_TracepointSource].support + == PACKET_ENABLE) + { + if (b->addr_string) { - if (b->addr_string) - { - strcpy (buf, "QTDPsrc:"); - encode_source_string (b->number, loc->address, - "at", b->addr_string, buf + strlen (buf), - 2048 - strlen (buf)); + strcpy (buf, "QTDPsrc:"); + encode_source_string (b->number, loc->address, + "at", b->addr_string, buf + strlen (buf), + 2048 - strlen (buf)); - putpkt (buf); - remote_get_noisy_reply (&target_buf, &target_buf_size); - if (strcmp (target_buf, "OK")) - warning (_("Target does not support source download.")); - } - if (b->cond_string) - { - strcpy (buf, "QTDPsrc:"); - encode_source_string (b->number, loc->address, - "cond", b->cond_string, buf + strlen (buf), - 2048 - strlen (buf)); - putpkt (buf); - remote_get_noisy_reply (&target_buf, &target_buf_size); - if (strcmp (target_buf, "OK")) - warning (_("Target does not support source download.")); - } - remote_download_command_source (b->number, loc->address, - breakpoint_commands (b)); + putpkt (buf); + remote_get_noisy_reply (&target_buf, &target_buf_size); + if (strcmp (target_buf, "OK")) + warning (_("Target does not support source download.")); } - - do_cleanups (old_chain); + if (b->cond_string) + { + strcpy (buf, "QTDPsrc:"); + encode_source_string (b->number, loc->address, + "cond", b->cond_string, buf + strlen (buf), + 2048 - strlen (buf)); + putpkt (buf); + remote_get_noisy_reply (&target_buf, &target_buf_size); + if (strcmp (target_buf, "OK")) + warning (_("Target does not support source download.")); + } + remote_download_command_source (b->number, loc->address, + breakpoint_commands (b)); } + + do_cleanups (old_chain); } static void @@ -10484,7 +10479,7 @@ Specify the serial device it is connected to\n\ remote_ops.to_supports_enable_disable_tracepoint = remote_supports_enable_disable_tracepoint; remote_ops.to_supports_string_tracing = remote_supports_string_tracing; remote_ops.to_trace_init = remote_trace_init; - remote_ops.to_download_tracepoint = remote_download_tracepoint; + remote_ops.to_download_tracepoint_loc = remote_download_tracepoint_loc; remote_ops.to_download_trace_state_variable = remote_download_trace_state_variable; remote_ops.to_enable_tracepoint = remote_enable_tracepoint; diff --git a/gdb/target.c b/gdb/target.c index 41ff6cf..c706f1d 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -674,7 +674,7 @@ update_current_target (void) INHERIT (to_supports_enable_disable_tracepoint, t); INHERIT (to_supports_string_tracing, t); INHERIT (to_trace_init, t); - INHERIT (to_download_tracepoint, t); + INHERIT (to_download_tracepoint_loc, t); INHERIT (to_download_trace_state_variable, t); INHERIT (to_enable_tracepoint, t); INHERIT (to_disable_tracepoint, t); @@ -847,8 +847,8 @@ update_current_target (void) de_fault (to_trace_init, (void (*) (void)) tcomplain); - de_fault (to_download_tracepoint, - (void (*) (struct breakpoint *)) + de_fault (to_download_tracepoint_loc, + (void (*) (struct bp_location *)) tcomplain); de_fault (to_download_trace_state_variable, (void (*) (struct trace_state_variable *)) diff --git a/gdb/target.h b/gdb/target.h index 352f2df..074515a 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -686,8 +686,10 @@ struct target_ops /* Prepare the target for a tracing run. */ void (*to_trace_init) (void); - /* Send full details of a tracepoint to the target. */ - void (*to_download_tracepoint) (struct breakpoint *t); + /* Send full details of a tracepoint location to the target. Target may + install or not install tracepoint locations to inferior, which is + determined by target's factors, such tracing state. */ + void (*to_download_tracepoint_loc) (struct bp_location *location); /* Send full details of a trace state variable to the target. */ void (*to_download_trace_state_variable) (struct trace_state_variable *tsv); @@ -1474,8 +1476,8 @@ extern int target_search_memory (CORE_ADDR start_addr, #define target_trace_init() \ (*current_target.to_trace_init) () -#define target_download_tracepoint(t) \ - (*current_target.to_download_tracepoint) (t) +#define target_download_tracepoint_loc(t) \ + (*current_target.to_download_tracepoint_loc) (t) #define target_download_trace_state_variable(tsv) \ (*current_target.to_download_trace_state_variable) (tsv) diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c index 4ca4ec2..493d324 100644 --- a/gdb/tracepoint.c +++ b/gdb/tracepoint.c @@ -1701,6 +1701,7 @@ start_tracing (void) for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++) { struct tracepoint *t = (struct tracepoint *) b; + struct bp_location *loc; if ((b->type == bp_fast_tracepoint ? !may_insert_fast_tracepoints @@ -1708,7 +1709,10 @@ start_tracing (void) continue; t->number_on_target = 0; - target_download_tracepoint (b); + + for (loc = b->loc; loc; loc = loc->next) + target_download_tracepoint_loc (loc); + t->number_on_target = b->number; } VEC_free (breakpoint_p, tp_vec); -- 1.7.0.4 --------------070003050107000002030707--