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

Jan Kratochvil
2012-04-11T18:43:01Z

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