From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32527 invoked by alias); 1 Mar 2013 15:51:03 -0000 Received: (qmail 32518 invoked by uid 22791); 1 Mar 2013 15:51:02 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,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 15:50:45 +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 r21Fohxv017292 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 1 Mar 2013 10:50:43 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r21FofOJ005212 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 1 Mar 2013 10:50:42 -0500 From: Tom Tromey To: Yao Qi Cc: Subject: Re: [PATCH 3/5] Read CTF by the ctf target References: <1361931459-3953-1-git-send-email-yao@codesourcery.com> <1361931459-3953-4-git-send-email-yao@codesourcery.com> Date: Fri, 01 Mar 2013 15:51:00 -0000 In-Reply-To: <1361931459-3953-4-git-send-email-yao@codesourcery.com> (Yao Qi's message of "Wed, 27 Feb 2013 10:17:37 +0800") Message-ID: <87txovunji.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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/msg00024.txt.bz2 >>>>> "Yao" == Yao Qi writes: Yao> +# Where is libbabeltrace? This will be empty if lzma was not available. Yao> +LIBBABELTRACE = @btlibs@ Yao> +LIBBABELTRACE_CFLAGS = @btinc@ This mentions "lzma", but I didn't see a link... maybe just a cut-and-pasto. It is clearer to use the same variable names in configure.ac and Makefile.in. I'd suggest @LIBBABELTRACE@ and @LIBBABELTRACE_CFLAGS@ and then change configure.ac. Yao> +AC_ARG_WITH(babeltrace, [ --with-babeltrace=PATH Specify prefix directory for the installed BABELTRACE package Yao> + Equivalent to --with-babeltrace-include=PATH/include Yao> + plus --with-babeltrace-lib=PATH/lib]) It's better to use AS_HELP_STRING. Yao> +AC_ARG_WITH(bt_include, [ --with-babeltrace-include=PATH Specify directory for installed babeltrace include files]) Lines are too long. AS_HELP_STRING will help with that :) Yao> +AC_ARG_WITH(bt_lib, [ --with-babeltrace-lib=PATH Specify the directory for the installed babeltrace library]) Yao> + Yao> +case $with_babeltrace in Yao> + no) Yao> + btlibs= Yao> + btinc= Yao> + ;; Yao> + "" | yes) Yao> + btlibs=" -lbabeltrace -lbabeltrace-ctf " Yao> + btinc="" Yao> + ;; Yao> + *) Yao> + btlibs="-L$with_babeltrace/lib -lbabeltrace -lbabeltrace-ctf" Yao> + btinc="-I$with_babeltrace/include " Yao> + ;; Yao> +esac Earlier code initialized the output variables, but it turns out there was no need to do that. Yao> + [AC_MSG_RESULT([yes]); AC_DEFINE(HAVE_LIBBABELTRACE, 1, [Define if libbabeltrace is available])], Needs some line breaks. Yao> +extern int trace_regblock_size; Maybe in tracepoint.h instead? I see other instances of this (in tracepoint.c and remote.c). In most cases I think extern declarations belong in headers. (There can be exceptions, IMO, but this doesn't seem like one.) Yao> + const struct bt_definition *scope Yao> + = bt_ctf_get_top_level_scope(event, Yao> + BT_EVENT_FIELDS); Yao> + const struct bt_definition *array Yao> + = bt_ctf_get_field(event, scope, "contents"); Yao> + Yao> + trace_regblock_size Yao> + = bt_ctf_get_array_len (bt_ctf_get_decl_from_def (array)); Some spaces missing before "(". Yao> + /* Restore the position. */ Yao> + bt_iter_set_pos(bt_ctf_get_iter (ctf_iter), pos); Likewise. There are more too, I didn't mark them all. Yao> + /* xfree (regs); */ It seems like this should be deleted. I don't really know, though. Yao> + unsigned short mlen; I'm curious about the rationale for the type here. And a bit later: Yao> + mlen = (uint16_t) bt_ctf_get_uint64 (def); Why use uint16_t here and not mlen's real type? But I don't understand why this has a shorter type anyhow. Yao> + /* It's unduly pedantic to refuse to look at the executable for Yao> + read-only pieces; so do the equivalent of readonly regions aka Yao> + QTro packet. */ Yao> + /* FIXME account for relocation at some point. */ There's a rule against adding new FIXME comments. Sometimes I think maybe we're too strict about this; though in practice it does seem to me that these are rarely fixed for their own sake, but instead just as a side effect of something else. I suppose you could rephrase it to be a note. Or is it fixable? It isn't clear to me in what situation this code is invoked. I was curious why this looks directly at the exec_bfd rather than using something like ALL_OBJFILE_OSECTIONS. It seemed like this might help with the relocation problem. Yao> + if (exec_bfd) A while ago we agreed to use the more explicit `exec_bfd != NULL' form. Yao> + /* Didn't find anything. */ Yao> + return found; I think this can be reached in the "did find something" case as well, so I suggest just dropping the comment. Yao> +/* Return what address the current frame was collected at. */ I think "Return the address at which the current frame was collected". Yao> + /* FIXME this is a poor heuristic if multiple locations. */ Yao> + if (tp && tp->base.loc) Yao> + addr = tp->base.loc->address; The FIXME rule again. Elsewhere the code just rejects multiple locations; but I'm not sure if that is appropriate here. Tom