From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10215 invoked by alias); 11 Apr 2012 18:43:01 -0000 Received: (qmail 10202 invoked by uid 22791); 11 Apr 2012 18:42:58 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 Apr 2012 18:42:44 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3BIgiOp023295 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 11 Apr 2012 14:42:44 -0400 Received: from host2.jankratochvil.net (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q3BIgYiP020654 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 11 Apr 2012 14:42:37 -0400 Date: Wed, 11 Apr 2012 19:06:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org, Pedro Alves , Tom Tromey , Mark Kettenis Subject: Re: [PATCH 2/4 v2] Implement new features needed for handling SystemTap probes Message-ID: <20120411184233.GA27572@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-04/txt/msg00260.txt.bz2 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 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 > + . > + > * 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 . */ > + > +#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