Re: [PATCH 2/4 v2] Implement new features needed for handling SystemTap probes

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

Hi Sergio,

$ gdb /lib64/ld-linux-x86-64.so.2
(gdb) info probes
Provider   Name       Where               Semaphore           Object
 - You have columns widths wrongly computed.
rtld       lll_futex_wake 0x000000000000ab2e                      /usr/lib/debug/lib64/ld-2.15.so.debug
rtld       lll_futex_wake 0x000000000000ab2e                      /usr/lib64/ld-linux-x86-64.so.2
[...]
 - You do not filter out separate debug info files or rather unify it somehow.


I see you have chosen abstraction (for non-stap probes) purely at the user
level, without abstraction at the GDB API level; if any non-Red Hat
contributor reads this mail what are your opinions?  I do not find great to
spread probe-backend (=stap) specific code across GDB.  breakpoint.c should
include (hypothetical) probe.h, not stap-probe.h.

Also the code like:
          val = bl->owner->ops->insert_location (bl);
+             stap_semaphore_up (bl->semaphore, bl->gdbarch);
suggests stap_semaphore_up should be done by virtualized
bkpt_stap_probe_breakpoint_ops->insert_location without such hack needed in
generic code.  The same applies to:
struct bp_location
+  CORE_ADDR semaphore;
there should be some generic void *user_data; for owner of the
breakpoint_location, when it is all already nicely virtualized by Pedro.

This part was not addressed, I leave it up to Tom, I guess he agrees with it so
OK, the values virtualization is not so great anyway, so fine with that:
# Moreover I would still more see to drop [patch 1/3], call just
# compute_probe_arg which returns lazy lval_computed struct value * which
# provides new struct lval_funcs member which can return `struct expression *'
# and generic code can call gen_expr on its own.  There is no need for special
# internalvar_funcs->compile_to_ax member.
# 
# internalvar_funcs->destroy can be also replaced by lval_funcs->free_closure.


On Fri, 06 Apr 2012 05:35:23 +0200, Sergio Durigan Junior wrote:
> definitions for a new command called `info probes'.  This command can
> take 2 arguments: `stap' and `all'.

'all' not, see in the code.


I was also looking at this code:
insert_exception_resume_from_probe:
  arg_value = stap_safe_evaluate_at_pc (frame, 1);
stap-probe.h:
/* A convenience function that finds a probe at the PC in FRAME and
   evaluates argument N.  If there is no probe at that location, or if
   the probe does not have enough arguments, this returns NULL.  */
extern struct value *stap_safe_evaluate_at_pc (struct frame_info *frame,
                                               int n);

and please describe that N is numbering 0 as the first argument and what is
that argument #1 in insert_exception_resume_from_probe.


> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,12 @@
>  
>  *** Changes since GDB 7.4
>  
> +* GDB now has support for SystemTap <sys/sdt.h> probes.  You can set a
> +  breakpoint using the new "-p" or "-probe" options and inspect the probe

It is called -probe-stap and -pstap.  (Although suggesting in the code
also/instead to provide -probe.)



> +  arguments using the new $_probe_arg family of convenience variables.
> +  You can obtain more information about SystemTap in
> +  <http://sourceware.org/systemtap/>.
> +
>  * GDB now supports reversible debugging on ARM, it allows you to
>    debug basic ARM and THUMB instructions, and provides 
>    record/replay support.  
[...]
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
[...]
> @@ -8962,6 +8978,14 @@ break_command_1 (char *arg, int flag, int from_tty)
>    enum bptype type_wanted = (flag & BP_HARDWAREFLAG
>  			     ? bp_hardware_breakpoint
>  			     : bp_breakpoint);
> +  struct breakpoint_ops *ops;
> +
> +  /* Matching breakpoints on SystemTap probes (`-pstap' or `-probe-stap').  */
> +  if (arg && ((strncmp (arg, "-probe-stap", 11) == 0 && isspace (arg[11]))
> +	      || (strncmp (arg, "-pstap", 6) == 0 && isspace (arg[6]))))

These options are not documented in 'help break' at all.

I miss there an option "-probe" which would break on any/all probe kind if
multiple backends exist, that was one of the points of the UI abstraction of
probes.


> +    ops = &bkpt_stap_probe_breakpoint_ops;
> +  else
> +    ops = &bkpt_breakpoint_ops;
>  
>    create_breakpoint (get_current_arch (),
>  		     arg,
[...]
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
[...]
> +static int
> +handle_probe (struct objfile *objfile, struct sdt_note *el,
> +	      struct stap_probe *ret, CORE_ADDR base)
> +{
> +  bfd *abfd = objfile->obfd;
> +  int size = bfd_get_arch_size (abfd) / 8;
> +  struct gdbarch *gdbarch = get_objfile_arch (objfile);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
> +  CORE_ADDR base_ref;
> +
> +  ret->gdbarch = gdbarch;
> +
> +  /* Provider and the name of the probe.  */
> +  ret->provider = &el->data[3 * size];
> +  ret->name = memchr (ret->provider, '\0',
> +		      (char *) el->data + el->size - ret->provider);
> +  /* Making sure there is a name.  */
> +  if (!ret->name)
> +    {
> +      complaint (&symfile_complaints, _("corrupt probe name when "
> +					"reading `%s'"), objfile->name);
> +
> +      /* There is no way to use a probe without a name or a provider, so
> +	 returning zero here makes sense.  */
> +      return 0;
> +    }
> +  else
> +    ++ret->name;
> +
> +  /* Retrieving the probe's address.  */
> +  ret->address = extract_typed_address (&el->data[0], ptr_type);
> +
> +  /* Link-time sh_addr of `.stapsdt.base' section.  */
> +  base_ref = extract_typed_address (&el->data[size], ptr_type);
> +
> +  /* Semaphore address.  */
> +  ret->sem_addr = extract_typed_address (&el->data[2 * size], ptr_type);
> +
> +  ret->address += (ANOFFSET (objfile->section_offsets,
> +			     SECT_OFF_TEXT (objfile))
> +		   + base - base_ref);
> +  if (ret->sem_addr)
> +    ret->sem_addr += (ANOFFSET (objfile->section_offsets,
> +				SECT_OFF_DATA (objfile))
> +		      + base - base_ref);
> +
> +  /* Arguments.  We can only extract the argument format if there is a valid
> +     name for this probe.  */
> +  ret->args = memchr (ret->name, '\0',
> +		      (char *) el->data + el->size - ret->name);

Here if ret->args == NULL you return a valid probe.  But in such case
ret->name is not properly '\0'-terminated and some code like compare_entries
may crash overrunning the memory as it just takes ret->name as a string.
Here if ret->args == NULL I believe you should also do that:
      complaint (&symfile_complaints, _("corrupt probe name when "
					"reading `%s'"), objfile->name);
      /* There is no way to use a probe without a name or a provider, so
	 returning zero here makes sense.  */
      return 0;

> +
> +  if (ret->args != NULL)
> +    ++ret->args;
> +
> +  if (ret->args == NULL || (memchr (ret->args, '\0',
> +				    (char *) el->data + el->size - ret->name)
> +			    != el->data + el->size - 1))
> +    {
> +      /* Although failing here is not good, it is still possible to use a
> +	 probe without arguments.  That's why we don't return zero.  */
> +      complaint (&symfile_complaints, _("corrupt probe argument when "
> +					"reading `%s'"), objfile->name);
> +      ret->args = NULL;
> +    }
> +
> +  return 1;
> +}
[...]
> +static int
> +get_base_address (bfd *obfd, bfd_vma *base)
> +{
> +  asection *ret = NULL;
> +
> +  bfd_map_over_sections (obfd, get_base_address_1, (void *) &ret);
> +
> +  if (!ret)
> +    {
> +      complaint (&symfile_complaints, _("could not obtain base address for "
> +					"SystemTap section."));

Not a requirement for change but in general please provide more specific
error/warning messages when it is easy to do, it could be said at more points,
such as here with obfd->name, when you load 100+ shared libraries and it
displays
	could not obtain base address for SystemTap section.
one may not be sure which library it was.


> +      return 0;
> +    }
> +
> +  if (base)
> +    *base = ret->vma;
> +
> +  return 1;
> +}
[...]
> --- /dev/null
> +++ b/gdb/probe.c
> @@ -0,0 +1,65 @@
> +/* Generic SDT probe support for GDB.

Not SDT.


> +
> +   Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +   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 "defs.h"
> +#include "stap-probe.h"
> +#include "command.h"
> +#include "cli/cli-cmds.h"
> +
> +/* Implementation of the `info probes' command.  */
> +
> +static void
> +info_probes_command (char *arg, int from_tty)
> +{
> +  /* If we are here, it means the user has not specified any
> +     argument, or has specified `all'.  In either case, we should
> +     print information about all types of probes.  */
> +  info_probes_stap_command (arg, from_tty);
> +}
> +
> +void _initialize_probe (void);
> +
> +void
> +_initialize_probe (void)
> +{
> +  static struct cmd_list_element *info_probes_cmdlist;
> +
> +  add_prefix_cmd ("probes", class_info, info_probes_command,
> +		  _("\
> +Show available static probes.\n\
> +Usage: info probes [all|TYPE [ARGS]]\n\

"info probes all" does not work.  "info probes" works correctly.

I do not think there is any "all" needed, just fix documentation that for all
kinds of probes one should type "info probes" and that's all, isn't it?


> +TYPE specifies the type of the probe, and can be one of the following:\n\
> +  - stap\n\
> +If you specify TYPE, there may be additional arguments needed by the\n\
> +subcommand.\n\
> +If you do not specify any argument, or specify `all', then the command\n\
> +will show information about all types of probes."),
> +		  &info_probes_cmdlist, "info probes ",
> +		  0/*allow-unknown*/, &infolist);
> +
> +  add_cmd ("stap", class_info, info_probes_stap_command,
> +	   _("\
> +Show information about SystemTap static probes.\n\
> +Usage: info probes stap [PROVIDER [NAME [OBJECT]]]\n\
> +Each argument is a regular expression, used to select probes.\n\
> +PROVIDER matches probe provider names.\n\
> +NAME matches the probe names.\n\
> +OBJECT matches the executable or shared library name."),
> +	   &info_probes_cmdlist);
> +}
[...]
> --- /dev/null
> +++ b/gdb/stap-probe.c
[...]
> +static void
> +stap_parse_probe_arguments (struct stap_probe *probe)
> +{
> +  struct stap_args_info *args_info;
> +  struct cleanup *back_to;
> +  const char *cur = probe->args;
> +  int current_arg = -1;
> +
> +  /* This is a state-machine parser, which means we will always be
> +     in a known state when parsing an argument.  The state could be
> +     either `NEW_ARG' if we are parsing a new argument, `BITNESS' if
> +     we are parsing the bitness-definition part (i.e., `4@'), or
> +     `PARSE_ARG' if we are actually parsing the argument part.  */
> +  enum
> +    {
> +      NEW_ARG,
> +      BITNESS,
> +      PARSE_ARG,
> +    } current_state;
> +
> +  /* For now, we assume everything is not going to work.  */
> +  probe->parsed_args = &dummy_stap_args_info;

I did not check if it is a regression due to stap_parse_argument throws an
exception now or not or if it is even intentional but I do not find it OK:

(gdb) file gdb.base/stap-probe
(gdb) break -pstap test:user
(gdb) run
(gdb) print $_probe_argc
stap_parse_argument: <-4(%rbp)>
Cannot parse expression `foo'.
(gdb) print $_probe_argc
$1 = 0

IMO it should give an error in each case, shouldn't it?


> +
> +  if (!cur || !*cur || *cur == ':')
> +    return;
> +
> +  args_info = xmalloc (sizeof (struct stap_args_info));
> +  args_info->n_args = 0;
> +  back_to = make_cleanup (stap_free_args_info, args_info);
> +  args_info->args = xcalloc (STAP_MAX_ARGS, sizeof (struct stap_probe_arg));
> +
> +  current_state = NEW_ARG;

Otherwise I am fine with the patch.


Thanks,
Jan