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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com]
> Sent: Wednesday, May 30, 2012 10:42 PM

Thanks for your review!

[...]

> > +/* A new thread observer enabling branch tracing for the new thread.
> > +*/ static struct observer *btrace_thread_observer;
> > +
> > +/* Branch tracing command list.  */
> > +static struct cmd_list_element *btcmdlist; static struct
> > +cmd_list_element *btencmdlist; static struct cmd_list_element
> > +*btdiscmdlist;
>
> The variables should be each commented.

Fixed.

> >  #if !defined(EALREADY)
> >  /* Remap EALREADY for systems that do not define it, e.g. mingw.  */
> > @@ -252,3 +268,591 @@ next_btrace (struct thread_info *tinfo)
> >
> >    return VEC_index (btrace_block_s, btinfo->btrace,
> > btinfo->iterator);  }
> > +
>
> Function comment.

Fixed.


> > +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.


> > +{
> > +  observer_new_thread_ftype *tfun = arg;
>
> Empty line after declarations.

Fixed all occurences in this source file.

[...]

> > +      if (!pid)
> > +        pid = ptid_get_pid (tinfo->ptid);
> > +
> > +      error (_("Couldn't enable branch tracing for %d: %s"),
> > +             pid, safe_strerror (errcode));
>
> Just use %s and as argument: target_pid_to_str (tinfo)

Fixed.


> But enable_btrace should error on its own as I have recommended.

Will fix once the situation with gdbserver and shared code is clear.


[...]


> > +      error (_("Couldn't disable branch tracing for %d: %s"),
> > +             pid, safe_strerror (errcode));
> > +    }
>
> Likewise above.

Fixed.


[...]

> > +  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.


> > +      safe_do_enable_btrace (tinfo);
>
> Here is TINFO still passed as a parameter so it is not required to call
> switch_to_thread.

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.


> > +    }
> > +
> > +  return 0;
> > +}
> > +
> > +static void
> > +cmd_btrace_enable (char *args, int from_tty) {
> > +  if (args)
>
> So far I think GDB considers ARGS may be either NULL or "" as no-
> parameters.

Fixed.

>
> > +    {
> > +      ptid_t ptid = inferior_ptid;
>
> Again use save_inferior_ptid like I suggested before.

Fixed all occurrences in this file.


[...]

> > +    }
> > +  else
> > +    {
> > +      struct thread_info *tinfo = find_thread_ptid (inferior_ptid);
>
> Empty line after declarations.

Fixed.


> > +      if (!tinfo)
> > +        error (_("Couldn't enable branch tracing: no inferior thread."));
> > +      else
>
> 'else' is not needed, 'error' doe snot return.

Fixed all occurrences in this file.


[...]


> > +#define BTR_LIST_ADDRESS  (1 << 0)
> > +#define BTR_LIST_FUNCTION (1 << 1)
> > +#define BTR_LIST_LINE     (1 << 2)
> > +#define BTR_LIST_TOTAL    (1 << 3)
>
> It should be rather enum.  Each item needs its comment.

Fixed.


> > +
> > +static void
> > +do_btrace_list_item (unsigned int block, struct btrace_block *trace,
> > +                     unsigned int flags) {
> > +  struct gdbarch *gdbarch = get_current_arch ();
>
> gdbarch depends on TRACE, not on current frame.  Each frame can have
> different gdbarch.  But putting gdbarch into each TRACE is probably too
> expensive.
>
> I would guess target_gdbarch is more acceptable to use for printing
> CORE_ADDR.

Replaced this with target_arch.


> > +  struct symtab *symtab = NULL;
> > +  struct symbol *symbol = NULL;
> > +
> > +  if (!trace)
> > +    return;
>
> Cannot happen, remove or gdb_assert.

Removed.


[...]

> > +        {
> > +          struct minimal_symbol *msymbol =
> > +            lookup_minimal_symbol_by_pc (trace->begin);
>
> Empty line.

Fixed.


[...]

> > +static void
> > +do_btrace_list (VEC (btrace_block_s) *btrace, char *range,
> > +                unsigned int flags)
> > +{
> > +  struct gdbarch *gdbarch = get_current_arch ();
>
> Unused variable.

Fixed.

[...]

> > +  else if (!btrace)
> > +    error (_("No trace"));
>
> dot:
>        error (_("No trace."));

Rearranged the if so the error case is at the final else. Since the preceding 
then statements
do not return, I still need to put the error () under else.

[...]

> > +      error (_("No trace."));
> > +      return;
>
> Excessive return.

Fixed all occurrences in this file.


> > +    }
> > +
> > +  if (trace->end < trace->begin)
> > +    error (_("Bad trace: 0x%" BFD_VMA_FMT "x - 0x%" BFD_VMA_FMT
> "x."),
> > +           trace->begin, trace->end);
>
> It is CORE_ADDR, not bfd_vma.  Therefore use paddress.

Fixed. Changed this to a warning.


> > +
> > +  gdb_disassembly (gdbarch, current_uiout, 0, flags, -1,
> > +                   trace->begin, trace->end + 1); }
> > +
> > +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.


> > +
> > +  if (!thread)
> > +    {
> > +      error (_("No thread."));
> > +      return;
>
> Excessive return.

Fixed.


[...]

> > +      VEC (btrace_block_s) *btrace = NULL;
> > +      struct cleanup *cleanup;
> > +      struct get_number_or_range_state state;
> > +      unsigned int i;
> > +
> > +      cleanup = make_cleanup (xfree, btrace);
>
> You must use VEC_cleanup here, not xfree.

Fixed.


> > +      init_number_or_range (&state, args);
> > +      while (!state.finished)
> > +        {
> > +          int index = get_number_or_range (&state);
>
> Empty line after declarations.

Fixed.


> > +          if (!index)
> > +            {
> > +              error (_("Args must be numbers or '$' variables."));
> > +              return;
>
> Excessive return.

Fixed.


> > +            }
> > +
> > +          trace = read_btrace (thread, index - 1);
> > +          if (!trace)
> > +            continue;
> > +
> > +          VEC_safe_push (btrace_block_s, btrace, trace);
> > +        }
> > +
> > +      i = VEC_length (btrace_block_s, btrace);
> > +      while (i--)
> > +        do_btrace (VEC_index (btrace_block_s, btrace, i), flags);
> > +
> > +      do_cleanups (cleanup);
> > +      return;
>
> The function is a bit spaghetting, maybe you could split it for the 
> different
> cases.  Just optional.

I split the function into argument processing and trace printing.


> > +    }
> > +
> > +  if (args && *args)
> > +    {
> > +      error (_("Junk after argument: %s"), args);
> > +      return;
>
> Excessive return.

Fixed.


[...]

> > +  add_cmd ("branchtrace", class_btrace, NULL,
> > +      _("Recording a branch trace."), &cmdlist);
>
> Incorrect indentation, it would be (using tab):
>   add_cmd ("branchtrace", class_btrace, NULL,
>            _("Recording a branch trace."), &cmdlist); but you can:
>   add_cmd ("branchtrace", class_btrace, NULL, _("Recording a branch
> trace."),
>            &cmdlist);

Fixed.


> > +  add_prefix_cmd ("btrace", class_btrace, cmd_btrace, _("\
> > +Disassemble the selected branch trace block.\n\n\ With a /m modifier,
> > +source lines are included (if available).\n\ With a /r modifier, raw
> > +instructions in hex are included.\n\n\ Without arguments, selects the
> > +chronologically preceding block.\n\ With \"+[<n>]\" argument, selects
> > +the n-th chronologically preceding block.\n\ With \"-[<n>]\"
> > +argument, selects the n-th chronologically succeeding block.\n\ With
> > +one positive integer argument, selects the respective block.\n\ With
> > +a range argument \"<l>-<h>\", selects the h-th block and disassembles
> > +\ blocks in the range in reverse (i.e. original control flow) order.\n"),
> > +                  &btcmdlist, "btrace ", 1, &cmdlist);
>
> In many cases - you should use tab character instead of 8 spaces.

Fixed.

Do you happen to know how to tell emacs to use tab for indentation?


[...]

> >      "List of classes of commands:"
> >      ""
> >      "aliases -- Aliases of other commands"
> > +    "branchtrace -- Recording a branch trace"
>
> I am not sure if it is worth a new class but it does not fit much in 
> 'tracepoints'.

It didn't fit anywhere, really, so I ended up adding a new class.

Regards,
Markus.

Attachment: smime.p7s
Description: S/MIME cryptographic signature

--------------------------------------------------------------------------------------
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland 
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer
Registergericht: Muenchen HRB 47456 
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052