Re: [PATCH 04/16] thread, btrace: add generic branch trace support

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

On Wed, 23 May 2012 13:22:19 +0200, markus.t.metzger@intel.com wrote:
> --- /dev/null
> +++ b/gdb/btrace.c
> @@ -0,0 +1,254 @@
> +/* Branch trace support for GDB, the GNU debugger.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "btrace.h"
> +#include "gdbthread.h"
> +#include "frame.h"
> +#include "exceptions.h"
> +
> +#include <errno.h>
> +
> +#if !defined(EALREADY)

GNU Coding Style formatting would be:
	#if !defined (EALREADY)
but maybe it is easier:
	#ifndef EALREADY


> +/* Remap EALREADY for systems that do not define it, e.g. mingw.  */
> +#  define EALREADY EBUSY
> +#endif
> +
> +int
> +enable_btrace (struct thread_info *tinfo)

Very every new function must have a comment before.
It is very common across the whole patchset.

This function is documented in btrace.h so it is enough to write here:

/* See definition in btrace.h.  */

int
enable_btrace (struct thread_info *tinfo)

I did not list them specifically but I see at least some of the new functions
without comment are really not documented in their corresponding .h file (at
least the 'static' ones).

Also use some common prefix for the global functions in btrace.c, most
probably just rename this function to btrace_enable and other functions too.


> +{
> +  if (!tinfo)
> +    return EINVAL;

This cannot happen (similarly in other functions) in current code.  It could
be rather 'gdb_assert (tinfo != NULL);' but I think it can be even omitted.


Also 'struct thread_info *' is commonly called 'tp' in GDB.  But it is not
required to change it.


> +
> +  if (tinfo->btrace.target)
> +    return EALREADY;

Isn't more suitable here to say some user message instead?
    error (_("Branch tracing is already enable for %s."),
           target_pid_to_str (tinfo->ptid));

The generic message is not too user friendly, in KVM (assuming not supporting
btrace that way):
	(gdb) btrace enable all
	warning: Couldn't enable branch tracing for 26535: No such file or directory

"No such file or directory" may not be understood well by a user.


> +
> +  tinfo->btrace.target = target_enable_btrace (tinfo->ptid);
> +  if (!tinfo->btrace.target)
> +    return (errno ? errno : ENOSYS);

I do not see the whole picture now but I do not find these error codes too
right, it is like in C code.  GDB uses more the
error()/throw_error()/TRY_CATCH exceptions.  Wouldn't it simplify the code
a lot?


> +
> +  return 0;
> +}
> +
> +int
> +disable_btrace (struct thread_info *tinfo)
> +{
> +  struct btrace_thread_info *btinfo;
> +  int errcode = 0;
> +
> +  if (!tinfo)
> +    return EINVAL;
> +
> +  btinfo = &tinfo->btrace;
> +
> +  if (!btinfo->target)
> +    return EALREADY;
> +
> +  /* When killing the inferior, we may have lost our target before we disable
> +     branch tracing.  */
> +  if (target_supports_btrace ())
> +    errcode = target_disable_btrace (btinfo->target);
> +
> +  if (!errcode)
> +    {
> +      VEC_free (btrace_block_s, btinfo->btrace);
> +      btinfo->btrace = NULL;

VEC_free already does 'btinfo->btrace = NULL;' (I agree it is confusing).


> +      btinfo->target = NULL;
> +    }
> +
> +  return errcode;
> +}
> +
> +static int
> +do_disconnect_btrace (struct thread_info *tinfo, void *ignored)
> +{
> +  if (tinfo->btrace.target)
> +    {
> +      volatile struct gdb_exception error;
> +
> +      TRY_CATCH (error, RETURN_MASK_ERROR)
> +        {
> +          /* Switching threads makes it easier for targets like kgdb, where we
> +             need to switch cpus, as well.  */
> +          switch_to_thread (tinfo->ptid);
> +
> +          disable_btrace (tinfo);
> +        }
> +    }
> +
> +  return 0;
> +}
> +
> +void
> +disconnect_btrace (void)
> +{
> +  ptid_t ptid = inferior_ptid;

Minor style issue - instead:
  struct cleanup *old_chain = save_inferior_ptid ();

> +
> +  iterate_over_threads (do_disconnect_btrace, NULL);
> +
> +  switch_to_thread (ptid);

Minor style issue - instead:
  do_cleanups (old_chain);

This makes it safe against possible future throws of errors.

> +}
> +
> +static struct btrace_thread_info *
> +get_btinfo (struct thread_info *tinfo)
> +{
> +  if (!tinfo)
> +    {
> +      errno = EINVAL;
> +      return NULL;
> +    }
> +  return &tinfo->btrace;
> +}
> +
> +static int
> +update_btrace (struct btrace_thread_info *btinfo)
> +{
> +  if (!btinfo)
> +    return EINVAL;
> +
> +  if (btinfo->target && target_btrace_has_changed (btinfo->target))
> +    {
> +      btinfo->btrace = target_read_btrace (btinfo->target);
> +      btinfo->iterator = -1;
> +
> +      if (!btinfo->btrace)
> +        return (errno ? errno : ENOSYS);
> +
> +      /* The first block ends at the current pc.  */
> +      if (!VEC_empty (btrace_block_s, btinfo->btrace))
> +        {
> +          struct frame_info *frame = get_current_frame ();

Empty line after declarations.

> +          if (frame)
> +            {
> +              struct btrace_block *head =
> +                VEC_index (btrace_block_s, btinfo->btrace, 0);

Empty line after declarations.

> +              if (head && !head->end)
> +                head->end = get_frame_pc (frame);
> +            }
> +        }
> +    }
> +
> +  return 0;
> +}
> +
> +VEC (btrace_block_s) *
> +get_btrace (struct thread_info *tinfo)
> +{
> +  struct btrace_thread_info *btinfo = get_btinfo (tinfo);
> +  int errcode;
> +
> +  if (!btinfo)
> +    return NULL;
> +
> +  errcode = update_btrace (btinfo);
> +  if (errcode)
> +    {
> +      errno = errcode;
> +      return NULL;
> +    }
> +  return btinfo->btrace;
> +}
> +
> +struct btrace_block *
> +read_btrace (struct thread_info *tinfo, int index)
> +{
> +  struct btrace_thread_info *btinfo = get_btinfo (tinfo);
> +  int errcode;
> +
> +  if (!btinfo)
> +    return NULL;
> +
> +  if (index < 0)
> +    {
> +      errno = EINVAL;
> +      return NULL;
> +    }
> +
> +  errcode = update_btrace (btinfo);
> +  if (errcode)
> +    {
> +      errno = errcode;
> +      return NULL;
> +    }
> +
> +  btinfo->iterator = index;
> +
> +  if (btinfo->iterator >= VEC_length (btrace_block_s, btinfo->btrace))
> +    {
> +      btinfo->iterator = VEC_length (btrace_block_s, btinfo->btrace);
> +      return NULL;

So == VEC_length is not permitted and you still set it to VEC_length?
Shouldn't it be set to VEC_length -1 in such case?  See more by comment for
the btrace_thread_info.iterator field.


> +    }
> +
> +  return VEC_index (btrace_block_s, btinfo->btrace, btinfo->iterator);
> +}
> +
> +struct btrace_block *
> +prev_btrace (struct thread_info *tinfo)
> +{
> +  struct btrace_thread_info *btinfo = get_btinfo (tinfo);
> +  int errcode;
> +
> +  if (!btinfo)
> +    return NULL;
> +
> +  errcode = update_btrace (btinfo);
> +  if (errcode)
> +    {
> +      errno = errcode;
> +      return NULL;
> +    }
> +
> +  btinfo->iterator += 1;
> +
> +  if (btinfo->iterator >= VEC_length (btrace_block_s, btinfo->btrace))
> +    {
> +      btinfo->iterator = VEC_length (btrace_block_s, btinfo->btrace);
> +      return NULL;
> +    }
> +
> +  return VEC_index (btrace_block_s, btinfo->btrace, btinfo->iterator);
> +}
> +
> +struct btrace_block *
> +next_btrace (struct thread_info *tinfo)
> +{
> +  struct btrace_thread_info *btinfo = get_btinfo (tinfo);
> +  int errcode;
> +
> +  if (!btinfo)
> +    return NULL;
> +
> +  errcode = update_btrace (btinfo);
> +  if (errcode)
> +    {
> +      errno = errcode;
> +      return NULL;
> +    }
> +
> +  btinfo->iterator -= 1;
> +
> +  if (btinfo->iterator < 0)
> +    {
> +      btinfo->iterator = -1;
> +      return NULL;
> +    }
> +
> +  return VEC_index (btrace_block_s, btinfo->btrace, btinfo->iterator);
> +}
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> new file mode 100644
> index 0000000..97f0f52
> --- /dev/null
> +++ b/gdb/btrace.h
> @@ -0,0 +1,89 @@
> +/* Branch trace support for GDB, the GNU debugger.
> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +   Contributed by Intel Corp. <markus.t.metzger@intel.com>.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef BTRACE_H
> +#define BTRACE_H
> +
> +#include "vec.h"
> +#include "defs.h"
> +
> +struct thread_info;
> +
> +/* A branch trace block.
> +
> +   Beware that a block is not a branch.  Rather, blocks are connected through
> +   branches.  */
> +struct btrace_block
> +{
> +  CORE_ADDR begin;
> +  CORE_ADDR end;

Describe END is the last byte (and not one-after-the-last-one).  BTW I would
find easier to make it rather one-after-the-last-byte.

> +};
> +
> +/* Branch trace is represented as a vector of branch trace blocks starting with
> +   the most recent block.  */
> +typedef struct btrace_block btrace_block_s;
> +DEF_VEC_O (btrace_block_s);
> +
> +/* Target specific branch trace information.  */
> +struct btrace_target_info;
> +
> +/* Branch trace information per thread.  */
> +struct btrace_thread_info
> +{
> +  /* The target branch trace information for this thread.  */
> +  struct btrace_target_info *target;
> +
> +  /* The current branch trace for this thread.  */
> +  VEC (btrace_block_s) *btrace;
> +
> +  /* The current iterator position in the above trace vector.  */
> +  int iterator;

Could you describe here what does mean if it is -1 and what does mean if it is
VEC_length (btrace)?  The code is doing some magic with it.


> +};
> +
> +/* Enable branch tracing for a thread.
> +   Allocates the branch trace target information.
> +   Returns 0 on success and an error value, otherwise.  */
> +extern int enable_btrace (struct thread_info *);
> +
> +/* Disable branch tracing for a thread.
> +   Deallocates the branch trace target information as well as the current branch
> +   trace data.
> +   Returns 0 on success and an error value, otherwise.  */
> +extern int disable_btrace (struct thread_info *);
> +
> +/* Disconnect branch tracing on detach.  */
> +extern void disconnect_btrace (void);
> +
> +/* Return the current branch trace vector for a thread, or NULL if ther is no
> +   trace.  */
> +extern VEC (btrace_block_s) *get_btrace (struct thread_info *);
> +
> +/* Functions to iterate over a thread's branch trace.
> +   There is one global iterator per thread.  The iterator is reset implicitly
> +   when branch trace for this thread changes.
> +   On success, read_btrace sets the iterator to the returned trace entry.
> +   Returns the selected block or NULL if there is no trace or the iteratoris
> +   out of bounds.  */
> +extern struct btrace_block *read_btrace (struct thread_info *, int);
> +extern struct btrace_block *prev_btrace (struct thread_info *);
> +extern struct btrace_block *next_btrace (struct thread_info *);
> +
> +#endif /* BTRACE_H */
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index fb8de16..055bac0 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -28,6 +28,7 @@ struct symtab;
>  #include "frame.h"
>  #include "ui-out.h"
>  #include "inferior.h"
> +#include "btrace.h"
>  
>  /* Frontend view of the thread state.  Possible extensions: stepping,
>     finishing, until(ling),...  */
> @@ -225,6 +226,9 @@ struct thread_info
>    /* Function that is called to free PRIVATE.  If this is NULL, then
>       xfree will be called on PRIVATE.  */
>    void (*private_dtor) (struct private_thread_info *);
> +
> +  /* Branch trace information for this thread.  */
> +  struct btrace_thread_info btrace;
>  };
>  
>  /* Create an empty thread list, or empty the existing one.  */
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 1f7564a..f9299e9 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -56,6 +56,7 @@
>  #include "inf-loop.h"
>  #include "continuations.h"
>  #include "linespec.h"
> +#include "btrace.h"
>  
>  /* Functions exported for general use, in inferior.h: */
>  
> @@ -2680,6 +2681,7 @@ detach_command (char *args, int from_tty)
>      error (_("The program is not being run."));
>  
>    disconnect_tracing (from_tty);
> +  disconnect_btrace ();
>  
>    target_detach (args, from_tty);
>  
> diff --git a/gdb/target.c b/gdb/target.c
> index 6dba936..4d2ea9e 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -701,6 +701,11 @@ update_current_target (void)
>        INHERIT (to_traceframe_info, t);
>        INHERIT (to_use_agent, t);
>        INHERIT (to_can_use_agent, t);
> +      INHERIT (to_supports_btrace, t);

This whole INHERIT / de_fault / '#define target_.*' way is the deprecated one.
The currently recommended way is to use stub functions like
target_verify_memory (and many others).  I am sorry it is probably not
documented anywhere.


> +      INHERIT (to_enable_btrace, t);
> +      INHERIT (to_disable_btrace, t);
> +      INHERIT (to_btrace_has_changed, t);
> +      INHERIT (to_read_btrace, t);
>        INHERIT (to_magic, t);
>        INHERIT (to_supports_evaluation_of_breakpoint_conditions, t);
>        /* Do not inherit to_memory_map.  */
> @@ -939,6 +944,21 @@ update_current_target (void)
>  	    (int (*) (void))
>  	    return_zero);
>    de_fault (to_execution_direction, default_execution_direction);
> +  de_fault (to_supports_btrace,
> +	    (int (*) (void))
> +	    return_zero);
> +  de_fault (to_enable_btrace,
> +	    (struct btrace_target_info * (*) (ptid_t))
> +	    tcomplain);
> +  de_fault (to_disable_btrace,
> +      (int (*) (struct btrace_target_info *))
> +	    tcomplain);
> +  de_fault (to_btrace_has_changed,
> +      (int (*) (struct btrace_target_info *))
> +	    tcomplain);
> +  de_fault (to_read_btrace,
> +      (VEC (btrace_block_s) * (*) (struct btrace_target_info *))
> +	    tcomplain);
>  
>  #undef de_fault
>  
> diff --git a/gdb/target.h b/gdb/target.h
> index 84b462a..86513f7 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -62,6 +62,7 @@ struct expression;
>  #include "memattr.h"
>  #include "vec.h"
>  #include "gdb_signals.h"
> +#include "btrace.h"
>  
>  enum strata
>    {
> @@ -849,6 +850,22 @@ struct target_ops
>      /* Is the target able to use agent in current state?  */
>      int (*to_can_use_agent) (void);
>  
> +    /* Check whether the target supports branch tracing.  */
> +    int (*to_supports_btrace) (void);
> +
> +    /* Enable branch tracing for @ptid and allocate a branch trace target
> +       information struct for reading and for disabling branch trace.  */
> +    struct btrace_target_info *(*to_enable_btrace) (ptid_t ptid);
> +
> +    /* Disable branch tracing. Deallocated @tinfo on success.  */
> +    int (*to_disable_btrace) (struct btrace_target_info *tinfo);
> +
> +    /* Check whether branch trace changed on the target.  */
> +    int (*to_btrace_has_changed) (struct btrace_target_info *);
> +
> +    /* Read branch trace data.  */
> +    VEC (btrace_block_s) *(*to_read_btrace) (struct btrace_target_info *);
> +
>      int to_magic;
>      /* Need sub-structure for target machine related rather than comm related?
>       */
> @@ -1710,6 +1727,21 @@ extern char *target_fileio_read_stralloc (const char *filename);
>  #define target_can_use_agent() \
>    (*current_target.to_can_use_agent) ()
>  
> +#define target_supports_btrace() \
> +  (*current_target.to_supports_btrace) ()
> +
> +#define target_enable_btrace(ptid) \
> +  (*current_target.to_enable_btrace) (ptid)
> +
> +#define target_disable_btrace(tinfo) \
> +  (*current_target.to_disable_btrace) (tinfo)
> +
> +#define target_btrace_has_changed(tinfo) \
> +  (*current_target.to_btrace_has_changed) (tinfo)
> +
> +#define target_read_btrace(tinfo) \
> +  (*current_target.to_read_btrace) (tinfo)
> +
>  /* Command logging facility.  */
>  
>  #define target_log_command(p)						\
> diff --git a/gdb/thread.c b/gdb/thread.c
> index d361dd8..e234fff 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -34,6 +34,7 @@
>  #include "regcache.h"
>  #include "gdb.h"
>  #include "gdb_string.h"
> +#include "btrace.h"
>  
>  #include <ctype.h>
>  #include <sys/types.h>
> @@ -132,6 +133,8 @@ free_thread (struct thread_info *tp)
>  	xfree (tp->private);
>      }
>  
> +  disable_btrace (tp);
> +
>    xfree (tp->name);
>    xfree (tp);
>  }
> -- 
> 1.7.1


Thanks,
Jan