From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22318 invoked by alias); 1 Jun 2012 18:42:02 -0000 Received: (qmail 22259 invoked by uid 22791); 1 Jun 2012 18:42:01 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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 Jun 2012 18:41:48 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q51IfiFP013471 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 1 Jun 2012 14:41:44 -0400 Received: from host2.jankratochvil.net (ovpn-116-47.ams2.redhat.com [10.36.116.47]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q51Ifd6d008639 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 1 Jun 2012 14:41:41 -0400 Date: Fri, 01 Jun 2012 18:42:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "kettenis@gnu.org" , "gdb-patches@sourceware.org" , "markus.t.metzger@gmail.com" Subject: Re: [PATCH 05/16] cli, btrace: add btrace cli Message-ID: <20120601184138.GA1957@host2.jankratochvil.net> References: <1337772151-20265-1-git-send-email-markus.t.metzger@intel.com> <1337772151-20265-6-git-send-email-markus.t.metzger@intel.com> <20120530204212.GE20633@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: 2012-06/txt/msg00034.txt.bz2 On Thu, 31 May 2012 17:32:03 +0200, Metzger, Markus T wrote: > > > +static int > > > +thread_callback (struct thread_info *tinfo, void *arg) > > > > Despite it is static there are some wishes for GDB all the functions should > > have the filename prefix, therefore btrace_thread_callback here. > > I pity the maintainer of gdb/dwarf2-frame-tailcall.c;-) > > You mentioned this for another patch, already. If this is gdb style, I can > certainly name my functions accordingly. I just don't see this a lot in > other gdb files, and it results in odd and quite lengthy names. Leaving this for comments from others. > > But enable_btrace should error on its own as I have recommended. > > Will fix once the situation with gdbserver and shared code is clear. But this part is in gdb/btrace.c, there is 'error' always available. But I agree gdb/common/ currently cannot use it. > > > + if (number_is_in_list (range, tinfo->num)) > > > + { > > > + /* Switching threads makes it easier for targets like kgdb, where > > > we > > need > > > + to switch cpus, as well. */ > > > + switch_to_thread (tinfo->ptid); > > > > In such case it should be switched in the most bottom function. > > I can move it to the enable and disable functions. This will result in a few > extra switches back to the selected thread when processing a list of > threads. I hope that's OK. Yes, I find it better for future fixes of the underlying functions to accept tinfo/tp/ptid_t. > I originally did not intend to switch threads, at all. All the btrace > functions take the thread to operate on as explicit argument. > > When I later added btrace support to kgdb, I realized that I had to switch > cpu's in order to perform certain operations. I should really do this inside > kgdb, but it was so much easier just to switch threads in gdb. > > If you're not OK with this, I can remove all the thread switching and look for > a solution in kgdb. Yes, it should be fixed kgdb. I do not know the kgdb codebase, though. > > > +static void > > > +cmd_btrace (char *args, int from_tty) { > > > + struct thread_info *thread = find_thread_ptid (inferior_ptid); > > > + struct btrace_block *trace = NULL; > > > + int flags = 0; > > > > Use enum. > > Flags is a bit-vector. The enum just provides names for the bits. It does not matter, you can enumval1 | enumval2 as long as they do not share any bits. There is even specific pretty printer support for such case: commit eb28de4a09f016d3de7caccec781eb70995a4001 http://sourceware.org/bugzilla/show_bug.cgi?id=13281 gdb.python/py-pp-maint.c +enum flag_enum + { + FLAG_1 = 1, + FLAG_2 = 2, + FLAG_3 = 4, + ALL = FLAG_1 | FLAG_2 | FLAG_3 + }; Thanks, Jan