From patchwork Tue Apr 1 12:06:57 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 371 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx20.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id D04223600CF for ; Tue, 1 Apr 2014 05:07:07 -0700 (PDT) Received: by homiemail-mx20.g.dreamhost.com (Postfix, from userid 14314964) id 7D61240C56675; Tue, 1 Apr 2014 05:07:07 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx20.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx20.g.dreamhost.com (Postfix) with ESMTPS id 4EF1C40C56667 for ; Tue, 1 Apr 2014 05:07:07 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=yG4DffMg0xuLo4r7 IRy19RwTngpCo2/xaUZv/X/gOqEDLyb3hOleMrQ0ZNhn/b54mdEKe+mDDJkyCfUv p1JIpZP1pKRCgWDsihBLBQTp5ff3kb7AjN9vxAq/b/zUBx7ZRHsUJBy8aqGMWGKW 12VeU76M6UuK94eqw3o8lg/RKJc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=vTrIvhVHOn87dll+MfCJoT MXj3o=; b=onFRTKT6NDgCWXg9KXNWPSuuqAiODNiLNqZwTJcURsP1w/l0zKwhzi EOzvbDnkVKE6aGZSK5DGJF0YUfniTlIH+jucy/hcmLIaFsu4LmmS62A6c/ryvT+y qhUgdItbt/jzMidx4hbTWFm15Bhta/5+mWox8cNmdUVE8TQiwoIJU= Received: (qmail 27742 invoked by alias); 1 Apr 2014 12:07:04 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 27732 invoked by uid 89); 1 Apr 2014 12:07:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 01 Apr 2014 12:07:02 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s31C70KC014885 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 1 Apr 2014 08:07:00 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s31C6wqP020279; Tue, 1 Apr 2014 08:06:59 -0400 Message-ID: <533AABE1.8040101@redhat.com> Date: Tue, 01 Apr 2014 13:06:57 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH] Fix several "set remote foo-packet on/off" commands. References: <1396307414-2053-1-git-send-email-palves@redhat.com> <533A7E83.4070200@codesourcery.com> In-Reply-To: <533A7E83.4070200@codesourcery.com> X-DH-Original-To: gdb@patchwork.siddhesh.in Hi Yao, Thanks for the review. On 04/01/2014 09:53 AM, Yao Qi wrote: > On 04/01/2014 07:10 AM, Pedro Alves wrote: >> +/* Returns true if the multi-process extensions are in effect. */ >> +static int >> +remote_multi_process_p (struct remote_state *rs) >> +{ >> + return packet_support (PACKET_multiprocess_feature) == PACKET_ENABLE; >> +} > > Argument 'rs' is not needed. We can remove it. Yeah. (Though note the parameter was already there, I just moved the function further below at the same time I reimplemented it.) > However, your patch moves more gdb remote target states from > 'struct remote_state' to a global state. I feel that it has > conflict with the multi-target project. As part of that project, we'll definitely need to move the whole remote_protocol_packets global array to remote_state. This global array already exists. I've just moved a few more manually managed flags into this array. Note this is listed as a TODO in the MultiTarget project's wiki page. Therefore, I'm not really adding any conflict, as that work will already have to be done. When that is done, we'll likely end up with passing a 'struct remote_state *' pointer to the new packet_support function. And with that in mind, it just looks like unnecessary churn to remove the parameter from remote_multi_process_p now (and update all callers), only to add it back again. Do you agree? > Not sure it is part > of your plan to convert the global state to per-target state. I don't plan to work on it myself in the near future, but it's definitely the way to go. >> + >> + # Force-disable the InstallInTrace RSP feature. >> + gdb_test_no_output "set remote install-in-trace-packet off" >> + >> + # Set a tracepoint while a trace experiment is ongoing. >> + set test "set tracepoint on set_tracepoint" >> + gdb_test_multiple "${trace_type} set_tracepoint" $test { >> + -re "Target returns error code .* too far .*$gdb_prompt $" { >> + if [string equal $trace_type "ftrace"] { >> + # The target was unable to install the fast tracepoint >> + # (e.g., jump pad too far from tracepoint). >> + pass "$test (too far)" > > A nit here, a fast tracepoint is not installed due to "jump pad too far" > instead of "set remote install-in-trace-packet off". Do we want to emit > a PASS here? Oh, hmm. Copy-paste from the other function in the file, and I just didn't think about that part much. It actually makes no sense to even expect that the target sends back any error code. We've just disabled installing tracepoints while a trace experiment is running, so we shouldn't see any response from the target at all. > IMO, UNSUPPORTED is better. Agreed, though note you made this a PASS in tracepoint_change_loc_1. ;-) > Secondly, if "jump pad too far" > causes fast tracepoint not installed, we should return here to skip the > rest of the test, because the test below can't tell why a fast > tracepoint is not installed, is it caused by > "set remote install-in-trace-packet off" or "jump pad too far". Even better is to not expect any error at all, and then make sure that no traceframe was collected. Here's what I'm folding into v2 of the patch. --- gdb/remote.c | 1 + gdb/testsuite/gdb.trace/change-loc.exp | 35 +++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 1aac5e3..dcd3cdd 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -1432,6 +1432,7 @@ show_remote_protocol_Z_packet_cmd (struct ui_file *file, int from_tty, } /* Returns true if the multi-process extensions are in effect. */ + static int remote_multi_process_p (struct remote_state *rs) { diff --git a/gdb/testsuite/gdb.trace/change-loc.exp b/gdb/testsuite/gdb.trace/change-loc.exp index ff6918a..d1a7aee 100644 --- a/gdb/testsuite/gdb.trace/change-loc.exp +++ b/gdb/testsuite/gdb.trace/change-loc.exp @@ -286,7 +286,7 @@ proc tracepoint_change_loc_2 { trace_type } { # doesn't work when we force-disable the InstallInTrace RSP feature. proc tracepoint_install_in_trace_disabled { trace_type } { - with_test_prefix "3 $trace_type" { + with_test_prefix "InstallInTrace disabled: $trace_type" { global testfile global srcfile global pcreg @@ -312,27 +312,18 @@ proc tracepoint_install_in_trace_disabled { trace_type } { "Tracepoint \[0-9\] at.* file .*$srcfile, line.*" \ "set tracepoint on main" + gdb_test "break marker" "Breakpoint.*at.* file .*$srcfile, line.*" \ + "breakpoint on marker" + gdb_test_no_output "tstart" # Force-disable the InstallInTrace RSP feature. gdb_test_no_output "set remote install-in-trace-packet off" # Set a tracepoint while a trace experiment is ongoing. - set test "set tracepoint on set_tracepoint" - gdb_test_multiple "${trace_type} set_tracepoint" $test { - -re "Target returns error code .* too far .*$gdb_prompt $" { - if [string equal $trace_type "ftrace"] { - # The target was unable to install the fast tracepoint - # (e.g., jump pad too far from tracepoint). - pass "$test (too far)" - } else { - fail $test - } - } - -re "\r\n$gdb_prompt $" { - pass $test - } - } + gdb_test "${trace_type} set_tracepoint" \ + "racepoint .* at .* set_tracepoint.*" \ + "set tracepoint on set_tracepoint" gdb_trace_setactions "set action for tracepoint" "" \ "collect \$$pcreg" "^$" @@ -340,8 +331,16 @@ proc tracepoint_install_in_trace_disabled { trace_type } { # Make sure the tracepoint is _not_ installed on the target. gdb_test "info trace" \ "Num Type\[ \]+Disp Enb Address\[ \]+What.* -\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*\.*3\.1.* in func4.*not installed on target.*3\.2.* in func4.*not installed on target.*" \ - "tracepoint with two locations" +\[0-9\]+\[\t \]+\(|fast \)tracepoint\[ \]+keep y.*installed on target.*\.*4\.1.* in func4.*not installed on target.*4\.2.* in func4.*not installed on target.*" \ + "tracepoint is not installed" + + gdb_test "continue" ".*Breakpoint.*marker.*at.*$srcfile.*" \ + "continue to marker" + + gdb_test_no_output "tstop" + + # Nothing should have been collected. + gdb_test "tfind" "Target failed to find requested trace frame\\..*" } }