From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32278 invoked by alias); 11 Apr 2012 21:33:46 -0000 Received: (qmail 32266 invoked by uid 22791); 11 Apr 2012 21:33:45 -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 21:33:29 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3BLXSb6021103 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 11 Apr 2012 17:33:28 -0400 Received: from psique (ovpn-112-65.phx2.redhat.com [10.3.112.65]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q3BLXJ8n020939; Wed, 11 Apr 2012 17:33:22 -0400 From: Sergio Durigan Junior To: Jan Kratochvil 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 References: <20120411184233.GA27572@host2.jankratochvil.net> X-URL: http://www.redhat.com Date: Wed, 11 Apr 2012 22:14:00 -0000 In-Reply-To: <20120411184233.GA27572@host2.jankratochvil.net> (Jan Kratochvil's message of "Wed, 11 Apr 2012 20:42:33 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg00264.txt.bz2 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 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 . */ >> + >> +#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