Messages in this thread | | | Date | Sun, 05 Oct 2014 13:47:45 +0530 | From | Hemant Kumar <> | Subject | Re: [PATCH v2 1/5] perf/sdt: ELF support for SDT |
| |
On 10/03/2014 05:09 PM, Masami Hiramatsu wrote: > (2014/10/01 11:47), Hemant Kumar wrote: >> This patch serves as the basic support to identify and list SDT events in binaries. >> When programs containing SDT markers are compiled, gcc with the help of assembler >> directives identifies them and places them in the section ".note.stapsdt". To find >> these markers from the binaries, one needs to traverse through this section and >> parse the relevant details like the name, type and location of the marker. Also, >> the original location could be skewed due to the effect of prelinking. If that is >> the case, the locations need to be adjusted. >> >> The functions in this patch open a given ELF, find out the SDT section, parse the >> relevant details, adjust the location (if necessary) and populate them in a list. >> >> Made the necessary changes as suggested by Namhyung Kim. > Looks almost good! > I have some comments, please see below.
Ok.
>> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com> >> --- >> tools/perf/util/symbol-elf.c | 207 ++++++++++++++++++++++++++++++++++++++++++ >> tools/perf/util/symbol.h | 19 ++++ >> 2 files changed, 226 insertions(+) >> >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c >> index 2a92e10..9702167 100644 >> --- a/tools/perf/util/symbol-elf.c >> +++ b/tools/perf/util/symbol-elf.c >> @@ -1672,6 +1672,213 @@ void kcore_extract__delete(struct kcore_extract *kce) >> unlink(kce->extract_filename); >> } >> >> +/* >> + * populate_sdt_note() : Responsible for parsing the section .note.stapsdt and >> + * after adjusting the note's location, returns that to the calling functions. >> + */ >> +static int populate_sdt_note(Elf **elf, const char *data, size_t len, int type, >> + struct sdt_note **note) >> +{ >> + const char *provider, *name; >> + struct sdt_note *tmp = NULL; >> + GElf_Ehdr ehdr; >> + GElf_Addr base_off = 0; >> + GElf_Shdr shdr; >> + int ret = -1; > -1 is not an error code. maybe, -EINVAL is better.
Sure, will change this to EINVAL.
>> + int i; >> + >> + union { >> + Elf64_Addr a64[3]; >> + Elf32_Addr a32[3]; >> + } buf; >> + >> + 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_err; >> + >> + tmp = (struct sdt_note *)calloc(1, sizeof(struct sdt_note)); >> + if (!tmp) { >> + ret = -ENOMEM; >> + goto out_err; >> + } >> + >> + INIT_LIST_HEAD(&tmp->note_list); >> + >> + if (len < dst.d_size + 3) >> + goto out_free_note; >> + >> + /* Translation from file representation to memory representation */ >> + if (gelf_xlatetom(*elf, &dst, &src, >> + elf_getident(*elf, NULL)[EI_DATA]) == NULL) >> + printf("gelf_xlatetom : %s\n", elf_errmsg(-1)); > pr_err? and shouldn't it goes to out_free_note?
Yes it should go to out_free_note. Will make the change.
>> [...] >> >>
Thank You for reviewing this patch.
-- Thanks, Hemant Kumar
| |