Re: [PATCH 05/16] cli, btrace: add btrace cli

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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