From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30988 invoked by alias); 1 Mar 2013 10:05:15 -0000 Received: (qmail 30973 invoked by uid 22791); 1 Mar 2013 10:05:13 -0000 X-SWARE-Spam-Status: No, hits=-8.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Mar 2013 10:05:08 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r21A51UF031378 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 1 Mar 2013 05:05:02 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r21A50cY026374; Fri, 1 Mar 2013 05:05:01 -0500 Message-ID: <51307D4C.5080805@redhat.com> Date: Fri, 01 Mar 2013 10:05:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Yao Qi CC: gdb-patches@sourceware.org Subject: Re: [PATCH 1/5] Refactor 'tsave' References: <1361931459-3953-1-git-send-email-yao@codesourcery.com> <1361931459-3953-2-git-send-email-yao@codesourcery.com> <512F86E0.7080508@redhat.com> <51306D5E.9050701@codesourcery.com> In-Reply-To: <51306D5E.9050701@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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: 2013-03/txt/msg00007.txt.bz2 On 03/01/2013 08:57 AM, Yao Qi wrote: > Hi Pedro, > thanks for the quick review. The updated patch address most of your comments except some I'd like to discuss with you below, > > On 03/01/2013 12:33 AM, Pedro Alves wrote: >> Ok, I think I got it. >> >> I think an important missing clue here is that "raw" in >> "write_raw_data" and target_get_raw_trace_data actually means >> frames in "tfile" format, right? "ops->write_raw_data" being >> NULL means that the writer can't handle parsing the "tfile" >> frames itself. >> >> "raw" here is almost meaningless to the reader -- surely "raw" in >> the ctf context could mean raw ctf "something". >> Maybe "raw" should really be s/raw/tfile/ or s/raw/raw_tfile/ ? >> > > "raw" here means the data in the trace buffers, and "raw data" has few to > do with the file format. And what is the format of that "raw" data? Exactly the format described in the manual, in the "Trace File Format" node: "The trace frame section consists of a number of consecutive frames. Each frame begins with a two-byte tracepoint number, followed by a four-byte size giving the amount of data in the frame. The data in the frame consists of a number of blocks, each introduced by a character indicating its type (at least register, memory, and trace state variable). The data in this section is raw binary, not a hexadecimal or other encoding; its endianness matches the target's endianness." So it's not really "raw" binary data -- there's a format, with headers and all. And it can't be any other format, otherwise the patch's design won't work... (**) > > TFILE is a format that composed by ascii definition part and trace frames dumped from the raw data directly. There could be another trace file format FOO that stores raw data as well. It's not "raw". It's trace frames in gdb's trace file format as defined in the manual. > >>> >+ { >>> >+ uint16_t tp_num; >>> >+ uint32_t tf_size; >>> >+ unsigned int read_length; >>> >+ unsigned int block; >>> >+ >>> >+ /* Read the first six bytes in, which is the tracepoint >>> >+ number and trace frame size. */ >>> >+ gotten = target_get_raw_trace_data (buf, offset, 6); >>> >+ >>> >+ if (gotten == 0) >>> >+ break; >>> >+ tp_num = (uint16_t) >>> >+ extract_unsigned_integer (&buf[0], 2, byte_order); >>> >+ >>> >+ tf_size = (uint32_t) >>> >+ extract_unsigned_integer (&buf[2], 4, byte_order); (**) ... as can be seen here and below with + switch (block_type) + { + case 'R': ... + case 'M': ... etc. This is parsing the "raw"'s headers, in gdb's trace file format. This as the else branch. The "then" branch only works if the target can interpret "buf" as trace frames in gdb's trace file format. >>> >+ >>> >+ writer->ops->frame_ops->start (writer, tp_num); >>> >+ gotten = 6; >>> >+ >>> >+ if (tf_size <= MAX_TRACE_UPLOAD) >>> >+ read_length = tf_size; >>> >+ else >>> >+ { >>> >+ read_length = MAX_TRACE_UPLOAD; >>> >+ gdb_assert (0); >> Leftover? I guess this means you didn't try with a big trace >> frame? >> > > I change this part that GDB will skip the frame if it is too big, and print a warning, like this: > > warning: Skip this trace frame because its size (708) is greater than the size of GDB internal buffer (200) > > I set MAX_TRACE_UPLOAD to 200, and run the testsuite. The CTF file is still correct, but some frames are lost. Ugh. Why do we have this limitation at all? target_get_raw_trace_data works with an offset and size, so I don't even see why we need to fetch the whole trace frame in one go. > >>> >+ } >>> >+ >>> >+ /* Write to mark the definition part is end. */ >> This doesn't parse correctly. Something like: >> >> /* Called to signal the end of the definition part. */ >> >> perhaps. >> > > Is it incorrect? I think so. > The basic sentence pattern is "write (an intransitive verb) to do sth.", and "[that] the definition part is end" is a noun clause. "to signal" is not intransitive. But I don't think an intransitive verb is what you wanted anyway. "is end" isn't correct. "is over", or "is done" or "is/has finished" or "has ended" would be good. By my order of preference: "Write to mark the end of the definition part." "Write to mark the definition part's end." "Write to mark the definition part has ended." Though "Write to mark" still sounds a little odd to me. It sounds to me like: "Write (anything at all you wish, unrelated to the definition part having ended. Perhaps a poem.) as celebration to mark the annual event that the definition part has ended. :-) But one of the above is good enough for me. Unless a native speaker chimes in, let's leave it at that. -- Pedro Alves