This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH v3 1/5] perf/sdt: ELF support for SDT
- From: Namhyung Kim <namhyung at kernel dot org>
- To: Hemant Kumar <hemant at linux dot vnet dot ibm dot com>
- Cc: linux-kernel at vger dot kernel dot org, srikar at linux dot vnet dot ibm dot com, peterz at infradead dot org, oleg at redhat dot com, hegdevasant at linux dot vnet dot ibm dot com, mingo at redhat dot com, anton at redhat dot com, systemtap at sourceware dot org, masami dot hiramatsu dot pt at hitachi dot com, aravinda at linux dot vnet dot ibm dot com, penberg at iki dot fi
- Date: Wed, 22 Oct 2014 11:39:48 +0900
- Subject: Re: [PATCH v3 1/5] perf/sdt: ELF support for SDT
- Authentication-results: sourceware.org; auth=none
- References: <20141010104402 dot 15506 dot 73285 dot stgit at hemant-fedora> <20141010105727 dot 15506 dot 99362 dot stgit at hemant-fedora>
Hi Hemant,
On Fri, 10 Oct 2014 16:27:53 +0530, Hemant Kumar wrote:
> This patch serves the initial 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.
>
> Signed-off-by: Hemant Kumar <hemant@linux.vnet.ibm.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Just a nitpick below..
> +static int construct_sdt_notes_list(Elf *elf, struct list_head *sdt_notes)
> +{
> + GElf_Ehdr ehdr;
> + Elf_Scn *scn = NULL;
> + Elf_Data *data;
> + GElf_Shdr shdr;
> + size_t shstrndx, next;
> + GElf_Nhdr nhdr;
> + size_t name_off, desc_off, offset;
> + int ret = 0;
> +
> + if (gelf_getehdr(elf, &ehdr) == NULL) {
> + ret = -EBADF;
> + goto out_ret;
> + }
> + if (elf_getshdrstrndx(elf, &shstrndx) != 0) {
> + ret = -EBADF;
> + goto out_ret;
> + }
> +
> + /* Look for the required section */
> + scn = elf_section_by_name(elf, &ehdr, &shdr, SDT_NOTE_SCN, NULL);
> + if (!scn) {
> + ret = -ENOENT;
> + goto out_ret;
> + }
> +
> + if (!(shdr.sh_type == SHT_NOTE) || (shdr.sh_flags & SHF_ALLOC)) {
I think the below is more readable:
if ((shdr.sh_type != SHT_NOTE) || (shdr.sh_flags & SHF_ALLOC)) {
Other than that, looks good to me!
Thanks,
Namhyung
> + ret = -ENOENT;
> + goto out_ret;
> + }
> +
> + data = elf_getdata(scn, NULL);
> +
> + /* Get the SDT notes */
> + for (offset = 0; (next = gelf_getnote(data, offset, &nhdr, &name_off,
> + &desc_off)) > 0; offset = next) {
> + if (nhdr.n_namesz == sizeof(SDT_NOTE_NAME) &&
> + !memcmp(data->d_buf + name_off, SDT_NOTE_NAME,
> + sizeof(SDT_NOTE_NAME))) {
> + ret = populate_sdt_note(&elf, ((data->d_buf) + desc_off),
> + nhdr.n_descsz, nhdr.n_type,
> + sdt_notes);
> + if (ret < 0)
> + goto out_ret;
> + }
> + }
> + if (list_empty(sdt_notes))
> + ret = -ENOENT;
> +
> +out_ret:
> + return ret;
> +}