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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

Thanks for the review.

On Wednesday, April 11 2012, Jan Kratochvil wrote:

> Hi Sergio,
>
> $ gdb /lib64/ld-linux-x86-64.so.2
> (gdb) info probes
> Provider   Name       Where               Semaphore           Object
>  - You have columns widths wrongly computed.

Thanks, I'll fix it.

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

`info probes stap' accept an argument specifying the provider, name and
objfile of the probe, so I guess this is the filter you're talking about.

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

As we have discussed, I will now try to implement this abstraction in a
proper way.  Meanwhile, I believe this patch is on-hold and probably not
valid for further reviews.

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

Thanks, I'll fix it.

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

Fixed.  I will still add the `-probe' option.

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

Ok, I will work on this.

>> --- 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;

Ok, fixed as you recommended.

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

Ok.

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

SDT stands for Statically Defined Probe, so I think SDT is correct in
this case.  Don't you?

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

I don't have a strong opinion about it, so yeah, we could use only
`info probes'.

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

I am taking a look, it should give a better error message.

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

-- 
Sergio