From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10967 invoked by alias); 8 Oct 2013 13:25:37 -0000 Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org Received: (qmail 10957 invoked by uid 89); 8 Oct 2013 13:25:37 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: e23smtp01.au.ibm.com Received: from e23smtp01.au.ibm.com (HELO e23smtp01.au.ibm.com) (202.81.31.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 08 Oct 2013 13:25:35 +0000 Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 8 Oct 2013 23:25:32 +1000 Received: from d23dlp01.au.ibm.com (202.81.31.203) by e23smtp01.au.ibm.com (202.81.31.207) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 8 Oct 2013 23:25:31 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id 907AA2CE8052 for ; Wed, 9 Oct 2013 00:25:30 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r98DPJGD6226376 for ; Wed, 9 Oct 2013 00:25:19 +1100 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r98DPT6s000929 for ; Wed, 9 Oct 2013 00:25:30 +1100 Received: from localhost.localdomain ([9.79.201.136]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id r98DP78w000472; Wed, 9 Oct 2013 00:25:20 +1100 Message-ID: <525407B2.9070401@linux.vnet.ibm.com> Date: Tue, 08 Oct 2013 13:25:00 -0000 From: Hemant User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Masami Hiramatsu CC: linux-kernel@vger.kernel.org, srikar@linux.vnet.ibm.com, peterz@infradead.org, oleg@redhat.com, hegdevasant@linux.vnet.ibm.com, mingo@redhat.com, anton@redhat.com, systemtap@sourceware.org, namhyung@kernel.org, aravinda@linux.vnet.ibm.com Subject: Re: [PATCH v2 1/3] SDT markers listing by perf: References: <20131007063911.11693.33624.stgit@hemant-fedora> <20131007064707.11693.27056.stgit@hemant-fedora> <5253EF03.5090502@hitachi.com> In-Reply-To: <5253EF03.5090502@hitachi.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13100813-1618-0000-0000-000004C101D3 X-SW-Source: 2013-q4/txt/msg00031.txt.bz2 On 10/08/2013 05:09 PM, Masami Hiramatsu wrote: > (2013/10/07 15:47), Hemant Kumar wrote: >> @@ -325,6 +326,7 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) >> opt_set_filter), >> OPT_CALLBACK('x', "exec", NULL, "executable|path", >> "target executable name or path", opt_set_target), >> + OPT_BOOLEAN('S', "markers", ¶ms.sdt, "Show probe-able sdt notes"), > BTW, "-S" for short of "--markers" option is a bit odd. Would we better use -M ? :) I thought of -S as SDT. :) Anyways, on second thought, will change that to -M. > > [...] >> @@ -355,6 +357,26 @@ int cmd_probe(int argc, const char **argv, const char *prefix __maybe_unused) >> */ >> symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL); >> >> + if (params.sdt) { >> + if (params.show_lines) { >> + pr_err("Error: Don't use --markers with --lines.\n"); >> + usage_with_options(probe_usage, options); >> + } >> + if (params.show_vars) { >> + pr_err("Error: Don't use --markers with --vars.\n"); >> + usage_with_options(probe_usage, options); >> + } >> + if (params.show_funcs) { >> + pr_err("Error: Don't use --markers with --funcs.\n"); >> + usage_with_options(probe_usage, options); >> + } >> + ret = show_sdt_notes(params.target); > You have to ensure params.target is set before use it. Yes, will do that. >> + if (ret < 0) { >> + pr_err(" Error : Failed to find SDT markers!" >> + "(%d)\n", ret); >> + } >> + return ret; >> + } >> if (params.list_events) { >> if (params.mod_events) { >> pr_err(" Error: Don't use --list with --add/--del.\n"); > [...] >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >> index 4b12bf8..6b17260 100644 >> --- a/tools/perf/util/symbol-elf.c >> +++ b/tools/perf/util/symbol-elf.c >> @@ -846,6 +846,182 @@ out_elf_end: >> return err; >> } >> >> +/* >> + * Populate the name, type, offset in the SDT note structure and >> + * ignore the argument fields (for now) >> + */ >> +static struct sdt_note *populate_sdt_note(Elf **elf, const char *data, >> + size_t len, int type) >> +{ >> + const char *provider, *name; >> + struct sdt_note *note; >> + >> + /* >> + * Three addresses need to be obtained : >> + * Marker location, address of base section and semaphore location >> + */ >> + union { >> + Elf64_Addr a64[3]; >> + Elf32_Addr a32[3]; >> + } buf; >> + >> + /* >> + * dst and src are required for translation from file to memory >> + * representation >> + */ >> + Elf_Data dst = { >> + .d_buf = &buf, .d_type = ELF_T_ADDR, .d_version = EV_CURRENT, >> + .d_size = gelf_fsize((*elf), ELF_T_ADDR, 3, EV_CURRENT), >> + .d_off = 0, .d_align = 0 >> + }; >> + >> + Elf_Data src = { >> + .d_buf = (void *) data, .d_type = ELF_T_ADDR, >> + .d_version = EV_CURRENT, .d_size = dst.d_size, .d_off = 0, >> + .d_align = 0 >> + }; >> + >> + /* Check the type of each of the notes */ >> + if (type != SDT_NOTE_TYPE) >> + goto out_ret; >> + >> + note = (struct sdt_note *)zalloc(sizeof(struct sdt_note)); >> + if (note == NULL) { >> + pr_err("Memory allocation error\n"); > No need to state out of memory for each place... > If you hit an out of memory for such small piece, you'd better > abort execution. For that purpose, I recommend you to return > errno from each function, and caller must check it. Yeah this will be better... will return errno from each function. >> + goto out_ret; >> + } >> + INIT_LIST_HEAD(¬e->note_list); >> + >> + if (len < dst.d_size + 3) >> + goto out_free; >> + >> + /* Translation from file representation to memory representation */ >> + if (gelf_xlatetom(*elf, &dst, &src, >> + elf_getident(*elf, NULL)[EI_DATA]) == NULL) >> + pr_debug("gelf_xlatetom : %s", elf_errmsg(-1)); >> + >> + /* Populate the fields of sdt_note */ >> + provider = data + dst.d_size; >> + >> + name = (const char *)memchr(provider, '\0', data + len - provider); >> + if (name++ == NULL) >> + goto out_free; >> + note->provider = strdup(provider); >> + note->name = strdup(name); >> + >> + /* Obtain the addresses and ignore notes with semaphores set*/ >> + if (gelf_getclass(*elf) == ELFCLASS32) { >> + note->addr.a32[0] = buf.a32[0]; >> + note->addr.a32[1] = buf.a32[1]; >> + note->addr.a32[2] = buf.a32[2]; >> + note->bit32 = true; >> + if (buf.a32[2] != 0) >> + goto out_free; >> + } else { >> + note->addr.a64[0] = buf.a64[0]; >> + note->addr.a64[1] = buf.a64[1]; >> + note->addr.a64[2] = buf.a64[2]; >> + note->bit32 = false; >> + if (buf.a64[2] != 0) >> + goto out_free; >> + } >> + return note; >> + >> +out_free: >> + free(note); >> +out_ret: >> + return NULL; >> +} >> + >> +static struct list_head *construct_sdt_notes_list(Elf *elf) > This also would better return errno. Ok. >> +{ >> + GElf_Ehdr ehdr; >> + Elf_Scn *scn = NULL; >> + Elf_Data *data; >> + GElf_Shdr shdr; >> + size_t shstrndx; >> + size_t next; >> + GElf_Nhdr nhdr; >> + size_t name_off, desc_off, offset; >> + struct sdt_note *tmp = NULL, *note = NULL; >> + >> + if (gelf_getehdr(elf, &ehdr) == NULL) { >> + pr_debug("%s: Can't get elf header.\n", __func__); >> + goto out_err; >> + } >> + if (elf_getshdrstrndx(elf, &shstrndx) != 0) { >> + pr_debug("getshdrstrndx failed\n"); >> + goto out_err; >> + } >> + >> + /* >> + * Look for section type = SHT_NOTE, flags = no SHF_ALLOC >> + * and name = .note.stapsdt >> + */ >> + scn = elf_section_by_name(elf, &ehdr, &shdr, NOTE_SCN, NULL); >> + if (!scn) { >> + printf("SDT markers not present!\n"); > Why you don't use pr_err() here ? :) Because, I thought it isn't an error that SDT markers are not present. It won't make much of a difference though. :) > And also, as I commented, I'd like to know which binary doesn't have SDT markers, > at least with -v (verbose) option. Yes, I guess it would be better to show the binary name even without -v option. Will make the changes. -- Thanks Hemant