From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11475 invoked by alias); 17 Apr 2013 15:48:14 -0000 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 Received: (qmail 11464 invoked by uid 89); 17 Apr 2013 15:48:14 -0000 X-Spam-SWARE-Status: No, score=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 17 Apr 2013 15:48:13 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3HFm9VO001244 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 17 Apr 2013 11:48:09 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r3HFm7Pk025241; Wed, 17 Apr 2013 11:48:08 -0400 Message-ID: <516EC437.5060400@redhat.com> Date: Wed, 17 Apr 2013 20:33:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: "Abid, Hafiz" CC: gdb-patches@sourceware.org Subject: Re: [patch] circ.exp References: <1366135406.4863.1@abidh-ubunto1104> In-Reply-To: <1366135406.4863.1@abidh-ubunto1104> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-04/txt/msg00537.txt.bz2 On 04/16/2013 07:03 PM, Abid, Hafiz wrote: > Thanks for the review and explnation about the circular behavior. > >> It'd have been useful if you had pointed out what were the issues. >> You must have come to the conclusion to you were going to refactor >> the code _after_ you've analyzed them. > Here are the problems that I observed. Thanks, that's very useful. > 3) When buffer is small and we can not have all 10 frames in it, it was testing > for "tfind 9" which tries to find the 10th frame which is never there due to small > buffer size. > I may be wrong but I think it should rather test for "tfind tracepoint > 9" which will give it a frame related to tracepoint 9. How interesting. The trace frame number is a detail of the target, actually. There's nothing in GDB (IIRC) that expects the first traceframe to be traceframe number 1. When you do "tfind 4", that asks the target to select traceframe 4 -- the 4 is sent to the target. It must have been that the original target that supported tracepoints, the one this test was written for originally years and years ago recorded the trace frame number in the traceframe itself. But that's not how GDBserver works. GDBserver counts traceframes from the first recorded traceframe instead: /* Given a traceframe number NUM, find the NUMth traceframe in the buffer. */ static struct traceframe * find_traceframe (int num) { struct traceframe *tframe; int tfnum = 0; for (tframe = FIRST_TRACEFRAME (); tframe->tpnum != 0; tframe = NEXT_TRACEFRAME (tframe)) { if (tfnum == num) return tframe; ++tfnum; } We should update the comments and test messages too then: # 1) frame zero will be overwritten and therefore unavailable # 2) the earliest frame in the buffer will be other-than-zero # 3) frame nine will be available (unlike "small buffer" case). s/other-than-zero/for a tracepoint other than 1/ s/frame nine/the frame for tracepoint 9/ gdb_test \ "tfind tracepoint 9" ".*Found trace frame $decimal, tracepoint 9.*" \ "find frame nine" s/frame /frame for tracepoint/ etc. Then might as well make all similar tfinds in the non-circular cases use "tfind tracepoint" too for consistency. > 4) Many tests were expecting current stack frame to be printed as result of > tfind command e.g. "#0 func9 .*". But this only happens when traceframe_number > is -1. In the following code, old_frame_id equal to current frame id unless > traceframe_number < 0. > I'm confused. Does the tfind in question switch to a different frame or not? > if (!(type == tfind_number && num == -1) > && (has_stack_frames () || traceframe_number >= 0)) > old_frame_id = get_frame_id (get_current_frame ()); > ... > if (frame_id_eq (old_frame_id, > get_frame_id (get_current_frame ()))) > print_what = SRC_LINE; > else > print_what = SRC_AND_LOC; >> As a follow up, it'd be nice to use gdb features/commands instead >> of manually sending packets to probe support. E.g, use tstatus to >> check for support: > I thought we meant to test both through commands and packets. But if you > feel that we should only use commands then I can do that in next version. I feel that we should only use commands. There's no good reason to limit the test to the remote protocol. As soon as someone implements tracepoints on some other target (native or something else), we'd need to stop using direct RSP packets anyway. The test used a direct packet because there was no user-accessible support for changing the trace buffer size. It was just a hack. Thanks. This is looking great. BTW, I realized this is the existing style in the test, but these "if (...) then return 1" sprinkled around are really unnecessary. + if [setup_tracepoints] then { return 1 } + if [gdb_test "break end" "Breakpoint $decimal.*" \ + "breakpoint at end"] then { return 1 } + if [gdb_test "tstart" \ + "\[\r\n\]*" \ + "start trace experiment"] then { return 1 } I'd zap them. -- Pedro Alves