From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22910 invoked by alias); 24 Feb 2012 13:13:40 -0000 Received: (qmail 22754 invoked by uid 22791); 24 Feb 2012 13:13:38 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00 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; Fri, 24 Feb 2012 13:13:24 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1S0uxf-0006rU-QJ from Yao_Qi@mentor.com ; Fri, 24 Feb 2012 05:13:23 -0800 Received: from SVR-ORW-FEM-05.mgc.mentorg.com ([147.34.97.43]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 24 Feb 2012 05:13:19 -0800 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.1.289.1; Fri, 24 Feb 2012 05:13:22 -0800 Message-ID: <4F478CB4.3060705@codesourcery.com> Date: Fri, 24 Feb 2012 13:19:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0) Gecko/20120130 Thunderbird/10.0 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [PATCH 6/9] agent capability of static tracepoint References: <1329447300-18841-1-git-send-email-yao@codesourcery.com> <1329447300-18841-7-git-send-email-yao@codesourcery.com> <4F46B4E2.6020504@redhat.com> In-Reply-To: <4F46B4E2.6020504@redhat.com> Content-Type: multipart/mixed; boundary="------------080806000706010008040609" 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: 2012-02/txt/msg00553.txt.bz2 --------------080806000706010008040609 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Content-length: 3114 On 02/24/2012 05:51 AM, Pedro Alves wrote: > Hmm, this looks backwards. We're reading the existence of a global in > the agent called "ust_loaded", indicating whether it has loaded > ust, and after, we check for the static trace capability. If > "ust_loaded" exists in the agent, then it sure understands static > tracepoints. The right check is: > > 1. does the agent understand static tracepoints? > 2. yes? good. and, is ust loaded perchance? > > If the agent doesn't understand AGENT_CAPA_STATIC_TRACE, > then you'd fail right on the ust_loaded read, or some other > mechanism to check whether ust is in fact loaded in the inferior. > This logic makes sense to me. >> > @@ -2315,6 +2319,10 @@ clear_installed_tracepoints (void) >> > ; >> > else >> > { >> > + /* Static tracepoints have been inserted, so agent should have >> > + been loaded and working. */ >> > + gdb_assert (in_process_agent_supports_ust ()); > This triggers an extra read off the inferior at each installed tracepoints. Is > it worth it? > Hmm, I am OK to remove it, to avoid reading from inferior. >> > @@ -2990,8 +2999,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf) >> > } >> > else >> > { >> > - if (tp) >> > - tpoint->handle = (void *) -1; > Why do we lose this? This was just cloning another static tracepoint, but > in the static tracepoint case, an installed static tracepoint has a handle == -1 > (vs NULL). > > Sorry, it is a mistake when I split patches. It should be in my next patch set, which refactor code here a little. >> > + if (!in_process_agent_supports_ust ()) >> > + warning ("Agent does not have capability for static tracepoint."); > How did we get so far then? There's that "Requested a static tracepoint, but static..." > check quoted above, above. > This part is redundant. Removed. >> > else > This if/else connection appears confused. > >> > { >> > if (probe_marker_at (tpoint->address, own_buf) == 0) >> > @@ -7994,6 +8003,8 @@ gdb_agent_helper_thread (void *arg) >> > #include >> > #include >> > >> > +IP_AGENT_EXPORT int gdb_agent_capability = AGENT_CAPA_STATIC_TRACE; >> > + >> > static void >> > gdb_agent_init (void) >> > { >> > diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c >> > index c56a02c..c2801f9 100644 >> > --- a/gdb/tracepoint.c >> > +++ b/gdb/tracepoint.c >> > @@ -4893,6 +4893,11 @@ info_static_tracepoint_markers_command (char *arg, int from_tty) >> > warning (_("Agent is off. Run `set agent on'.")); >> > return; >> > } >> > + if (!agent_capability_check (AGENT_CAPA_STATIC_TRACE)) >> > + { >> > + warning (_("Agent is not capable of operating static tracepoints")); >> > + return; >> > + } > Same comment as in the other patch. I don't think this is right. Also, does > this work for remote debugging? Who is calling agent_look_up_symbols? gdb > knowing about IPA's internals when remote debugging feels a bit dirty. > This chunk is removed, explained in my reply to patch 2/9. -- Yao (齐尧) --------------080806000706010008040609 Content-Type: text/x-patch; name="0006-agent-capability-of-static-tracepoint.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0006-agent-capability-of-static-tracepoint.patch" Content-length: 2662 gdb/gdbserver: 2012-02-24 Yao Qi * tracepoint.c (gdb_agent_capability): New global. (in_process_agent_loaded_ust): Renamed to `in_process_agent_supports_ust'. Update callers. (in_process_agent_supports_ust): Call agent_capability_check. (clear_installed_tracepoints): Assert that agent supports agent. --- gdb/gdbserver/tracepoint.c | 29 ++++++++++++++++++++--------- 1 files changed, 20 insertions(+), 9 deletions(-) diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c index 3b6f2f4..a48edaa 100644 --- a/gdb/gdbserver/tracepoint.c +++ b/gdb/gdbserver/tracepoint.c @@ -239,10 +239,11 @@ in_process_agent_loaded (void) static int read_inferior_integer (CORE_ADDR symaddr, int *val); /* Returns true if both the in-process agent library and the static - tracepoints libraries are loaded in the inferior. */ + tracepoints libraries are loaded in the inferior, and agent has + capability on static tracepoints. */ static int -in_process_agent_loaded_ust (void) +in_process_agent_supports_ust (void) { int loaded = 0; @@ -252,13 +253,20 @@ in_process_agent_loaded_ust (void) return 0; } - if (read_inferior_integer (ipa_sym_addrs.addr_ust_loaded, &loaded)) + if (agent_capability_check (AGENT_CAPA_STATIC_TRACE)) { - warning ("Error reading ust_loaded in lib"); - return 0; - } + /* Agent understands static tracepoint, then check whether UST is in + fact loaded in the inferior. */ + if (read_inferior_integer (ipa_sym_addrs.addr_ust_loaded, &loaded)) + { + warning ("Error reading ust_loaded in lib"); + return 0; + } - return loaded; + return loaded; + } + else + return 0; } static void @@ -310,7 +318,7 @@ maybe_write_ipa_ust_not_loaded (char *buffer) write_e_ipa_not_loaded (buffer); return 1; } - else if (!in_process_agent_loaded_ust ()) + else if (!in_process_agent_supports_ust ()) { write_e_ust_not_loaded (buffer); return 1; @@ -2965,7 +2973,8 @@ install_tracepoint (struct tracepoint *tpoint, char *own_buf) write_e_ipa_not_loaded (own_buf); return; } - if (tpoint->type == static_tracepoint && !in_process_agent_loaded_ust ()) + if (tpoint->type == static_tracepoint + && !in_process_agent_supports_ust ()) { trace_debug ("Requested a static tracepoint, but static " "tracepoints are not supported."); @@ -7988,6 +7997,8 @@ gdb_agent_helper_thread (void *arg) #include #include +IP_AGENT_EXPORT int gdb_agent_capability = AGENT_CAPA_STATIC_TRACE; + static void gdb_agent_init (void) { -- 1.7.0.4 --------------080806000706010008040609--