From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14029 invoked by alias); 27 Feb 2013 19:38:39 -0000 Received: (qmail 13967 invoked by uid 22791); 27 Feb 2013 19:38:38 -0000 X-SWARE-Spam-Status: No, hits=-6.6 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; Wed, 27 Feb 2013 19:38:31 +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 r1RJcUTe014042 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 27 Feb 2013 14:38:30 -0500 Received: from host2.jankratochvil.net (ovpn-116-19.ams2.redhat.com [10.36.116.19]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r1RJcP7C002135 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 27 Feb 2013 14:38:27 -0500 Date: Wed, 27 Feb 2013 19:43:00 -0000 From: Jan Kratochvil To: markus.t.metzger@intel.com Cc: gdb-patches@sourceware.org, markus.t.metzger@gmail.com Subject: Re: [PATCH 1/3] record, btrace: add record-btrace target Message-ID: <20130227193824.GA27661@host2.jankratochvil.net> References: <1361808917-16934-1-git-send-email-markus.t.metzger@intel.com> <1361808917-16934-2-git-send-email-markus.t.metzger@intel.com> <20130227073731.GA1378@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130227073731.GA1378@host2.jankratochvil.net> User-Agent: Mutt/1.5.21 (2010-09-15) 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: 2013-02/txt/msg00705.txt.bz2 On Wed, 27 Feb 2013 08:37:31 +0100, Jan Kratochvil wrote: [...] > > +/* Check if we should skip this file when generating the function call > > + history. > > + Update the recorded function segment. */ > > I do not see a reason for this functionality. There really may be a valid one > but I do not see it. Could you provide an example in the comment when it is > useful? I cannot much review it when I do not see what is it good for. > > > > + > > +static int > > +btrace_function_skip_file (struct btrace_function *bfun, > > + const char *filename) Probably if one does: file.c: void func(void) { #include "file.def" } Then we want a single output line for file.c:func() without another output line for file.def:func(). (It is not tested by the testsuite; OK.) Therefore you apparently do not want to include the line numbers range of file.def:func() into the range of lines of file.c:func(), that makes sense. So my comments are valid, just use filename_cmp and compare symtab_to_fullname of both symtabs (this is sub-optimal, comparing two source files for equivalence can be made cheaper than finding out their full pathname; but that is an optimization for another patch). > > +{ > > + /* Update the filename in case we didn't get it from the function symbol. */ > > + if (bfun->filename == NULL) > > + { > > + DEBUG_CALL ("file: '%s'", filename); > > + > > + bfun->filename = filename; > > + return 0; > > + } > > + > > + /* Check if we're still in the same file. */ > > + if (compare_filenames_for_search (bfun->filename, filename)) > > + return 0; > > Use filename_cmp for full pathnames. Therefore always use symtab_to_fullname. > The result of symtab_to_fullname call as 'const char *fullname'. > > > > + > > + /* We switched files, but not functions. Skip this file. */ > > + DEBUG_CALL ("ignoring file '%s' for %s in %s.", filename, > > + bfun->function, bfun->filename); > > + return 1; > > +} [...] > > + /* Let's see if we have source correlation, as well. */ > > + line = find_pc_line (ip, 0); > > Such variable is commonly called 'sal' (source-and-line) in GDB. > > > > + if (line.symtab != NULL) > > + { > > + const char *filename; > > + > > + /* Without a filename, we ignore this instruction. */ > > + filename = symtab_to_filename_for_display (line.symtab); > > + if (filename == NULL) > > + return; > > As you have non-NULL symtab here you always have non-NULL filename. > > You should use symtab_to_fullname here as discussed elsewhere. > > > > + > > + /* Do this check first to make sure we get a filename even if we don't > > + have line information. */ > > + if (btrace_function_skip_file (bfun, filename)) > > But I do not see a reason for this function as written at > btrace_function_skip_file. > > > > + return; > > + > > + if (line.line == 0) > > + return; > > + > > + btrace_function_update_lines (bfun, line.line, ip); > > + } > > +} Jan