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

Sergio Durigan Junior
2012-04-11T21:33:46Z

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