Re: [PATCH v2 1/5] perf/sdt: ELF support for SDT

[Hemant Kumar <hemant at linux dot vnet dot ibm dot com>]

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