Adding Roland to the CC list, perhaps he can help us with these debuginfo details :-) Em Mon, Jun 11, 2012 at 04:47:34PM +0530, Prashanth Nageshappa escreveu: > Dave/Arnaldo, > Let me know if you have any comments on this patch so that I can address them. > -------- Original Message -------- > Subject: [PATCH-V2] perf symbols: fix symbol offset breakage with separated debug info > Date: Wed, 02 May 2012 12:32:06 +0530 > From: Prashanth Nageshappa <prashanth@xxxxxxxxxxxxxxxxxx> > To: peterz@xxxxxxxxxxxxx, mingo@xxxxxxx > CC: akpm@xxxxxxxxxxxxxxxxxxxx, torvalds@xxxxxxxxxxxxxxxxxxxx, ananth@xxxxxxxxxx, jkenisto@xxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, linux-mm@xxxxxxxxx, oleg@xxxxxxxxxx, andi@xxxxxxxxxxxxxx, hch@xxxxxxxxxxxxx, rostedt@xxxxxxxxxxx, acme@xxxxxxxxxxxxx, masami.hiramatsu.pt@xxxxxxxxxxx, tglx@xxxxxxxxxxxxx, anton@xxxxxxxxxx, srikar@xxxxxxxxxxxxxxxxxx, dave.martin@xxxxxxxxxx, linux-perf-users@xxxxxxxxxxxxxxx > > From: Dave Martin <dave.martin@xxxxxxxxxx> > > perf resolves symbols to wrong offsets when debug info is separated > from the lib/executable. > > This patch is based on Dave Martin's initial work first published at > https://lkml.org/lkml/2010/11/22/137 > > This patch loads the ELF section headers from a separate file if > necessary, to avoid getting confused by the different section file > offsets seen in debug images. Invalid section headers are detected by > checking for the presence of non-writable SHT_NOBITS sections, which > don't make sense under normal circumstances. > > In particular, this allows symbols in ET_EXEC images to get fixed up > correctly in the presence of separated debug images. > > v2 addresses review comments (to remove asserts) from Dave Martin. > > Signed-off-by: Dave Martin <dave.martin@xxxxxxxxxx> > Signed-off-by: Prashanth Nageshappa <prashanth@xxxxxxxxxxxxxxxxxx> > --- > > tools/perf/util/symbol.c | 168 +++++++++++++++++++++++++++++++++++++++++++++- > 1 files changed, 164 insertions(+), 4 deletions(-) > > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c > index caaf75a..a6ad4b1 100644 > --- a/tools/perf/util/symbol.c > +++ b/tools/perf/util/symbol.c > @@ -1158,8 +1158,105 @@ static size_t elf_addr_to_index(Elf *elf, GElf_Addr addr) > return -1; > } > > +/** > + * Read all section headers, copying them into a separate array so they survive > + * elf_end. > + * > + * @elf: the libelf instance to operate on. > + * @ehdr: the elf header: this must already have been read with gelf_getehdr(). > + * @count: the number of headers read is assigned to *count on successful > + * return. count must not be NULL. > + * > + * Returns a pointer to the allocated headers, which should be deallocated with > + * free() when no longer needed. > + */ > +static GElf_Shdr *elf_get_all_shdrs(Elf *elf, GElf_Ehdr const *ehdr, > + unsigned *count) > +{ > + GElf_Shdr *shdrs; > + Elf_Scn *scn; > + unsigned max_index = 0; > + unsigned i; > + > + shdrs = malloc(ehdr->e_shnum * sizeof *shdrs); This could be zalloc so that... > + if (!shdrs) > + return NULL; > + > + for (i = 0; i < ehdr->e_shnum; i++) > + shdrs[i].sh_type = SHT_NULL; The loop above can be removed > + for (scn = NULL; (scn = elf_nextscn(elf, scn)); ) { > + size_t j; > + > + /* > + * Just assuming we get section 0, 1, ... in sequence may lead > + * to wrong section indices. Check the index explicitly: > + */ > + j = elf_ndxscn(scn); > + if (j > max_index) > + max_index = j; > + > + if (!gelf_getshdr(scn, &shdrs[j])) > + goto error; > + } > + > + *count = max_index + 1; > + return shdrs; > + > +error: > + free(shdrs); > + return NULL; > +} > + > +/** > + * Check that the section headers @shdrs reflect accurately the file data > + * layout of the image that was loaded during perf record. This is generally > + * not true for separated debug images generated with e.g., > + * objcopy --only-keep-debug. > + * > + * We identify invalid files by checking for non-empty sections which are > + * declared as having no file data (SHT_NOBITS) but are not writable. > + * > + * @shdrs: the full set of section headers, as loaded by elf_get_all_shdrs(). > + * @count: the number of headers present in @shdrs. > + * > + * Returns 1 for valid headers, 0 otherwise. > + */ Roland, could you take a look at these ELF/debuginfo aspects? > +static int elf_check_shdrs_valid(GElf_Shdr const *shdrs, unsigned count) > +{ > + unsigned i; > + > + for (i = 0; i < count; i++) { > + if (shdrs[i].sh_type == SHT_NOBITS && > + !(shdrs[i].sh_flags & SHF_WRITE) && > + shdrs[i].sh_size != 0) > + return 0; > + } > + > + return 1; > +} > + > +/* > + * Notes: > + * > + * If saved_shdrs is non-NULL, the section headers will be read if found, and > + * will be used for address fixups. saved_shdrs_count must also be non-NULL in > + * this case. This may be needed for separated debug images, since the section > + * headers and symbols may need to come from separate images in that case. > + * > + * Note: irrespective of whether this function returns successfully, > + * *saved_shdrs may get initialised if saved_shdrs is non-NULL. It is the > + * caller's responsibility to free() it when non longer needed. I kept deferring looking at this patch because of the added complexity, couldn't we try to simplify it somehow? > + * If want_symtab == 1, this function will only load symbols from .symtab > + * sections. Otherwise (want_symtab == 0), .dynsym or .symtab symbols are > + * loaded. This feature is used by dso__load() to search for the best image > + * to load. > + */ > + > static int dso__load_sym(struct dso *dso, struct map *map, const char *name, > int fd, symbol_filter_t filter, int kmodule, > + GElf_Shdr **saved_shdrs, unsigned *saved_shdrs_count, > int want_symtab) > { > struct kmap *kmap = dso->kernel ? map__kmap(map) : NULL; > @@ -1178,6 +1275,17 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name, > int nr = 0; > size_t opdidx = 0; > > + if (saved_shdrs != NULL && saved_shdrs_count == NULL) { > + /* > + * If you trigger this check, you're calling this function > + * incorrectly. Refer to the notes above for details. > + */ > + pr_debug("%s: warning: saved_shdrs_count == NULL: " > + "ignoring the saved section headers.\n", > + __func__); > + saved_shdrs = NULL; > + } Why check if one of the parameters is null? Perhaps we should have something like: struct saved_shdrs { GElf_Shdr **saved_shdrs; unsigned *saved_shdrs_count; } And pass just one parameter? > elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL); > if (elf == NULL) { > pr_debug("%s: cannot read %s ELF file.\n", __func__, name); > @@ -1200,6 +1308,36 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name, > goto out_elf_end; > } > > + /* > + * Copy all section headers from the image if requested and if not > + * already loaded. > + */ > + if (saved_shdrs != NULL && *saved_shdrs == NULL) { > + GElf_Shdr *shdrs; > + unsigned count; > + > + shdrs = elf_get_all_shdrs(elf, &ehdr, &count); > + if (shdrs == NULL) > + goto out_elf_end; > + > + /* > + * Only keep the headers if they reflect the actual run-time > + * image's file layout: > + */ > + if (elf_check_shdrs_valid(shdrs, count)) { > + *saved_shdrs = shdrs; > + *saved_shdrs_count = count; > + } else > + free(shdrs); > + } > + > + /* > + * If no genuine ELF headers are available yet, give up: we can't > + * adjust symbols correctly in that case: > + */ > + if (saved_shdrs != NULL && *saved_shdrs == NULL) > + goto out_elf_end; > sec = elf_section_by_name(elf, &ehdr, &shdr, ".symtab", NULL); > if (sec == NULL) { > if (want_symtab) > @@ -1344,12 +1482,25 @@ static int dso__load_sym(struct dso *dso, struct map *map, const char *name, > goto new_symbol; > } > > + /* > + * Currently, symbols for shared objects and PIE executables > + * (i.e., ET_DYN) do not seem to get adjusted. This might need > + * to change if file offset == virtual address is not actually > + * guaranteed for these images. ELF doesn't provide this > + * guarantee natively. > + */ > if (curr_dso->adjust_symbols) { > pr_debug4("%s: adjusting symbol: st_value: %#" PRIx64 " " > "sh_addr: %#" PRIx64 " sh_offset: %#" PRIx64 "\n", __func__, > (u64)sym.st_value, (u64)shdr.sh_addr, > (u64)shdr.sh_offset); > - sym.st_value -= shdr.sh_addr - shdr.sh_offset; > + if (saved_shdrs && *saved_shdrs && > + sym.st_shndx < *saved_shdrs_count) > + sym.st_value -= > + (*saved_shdrs)[sym.st_shndx].sh_addr - > + (*saved_shdrs)[sym.st_shndx].sh_offset; > + else > + sym.st_value -= shdr.sh_addr - shdr.sh_offset; All this couldn't be on a separate helper routine? > } > /* > * We need to figure out if the object was created from C++ sources > @@ -1590,6 +1741,8 @@ int dso__load(struct dso *dso, struct map *map, symbol_filter_t filter) > struct machine *machine; > const char *root_dir; > int want_symtab; > + GElf_Shdr *saved_shdrs = NULL; > + unsigned saved_shdrs_count;o Couldn't saved_shdrs be a member of struct dso so that we don't have to be passing parameters back and forth. These questions are more trying to simplify the patch, I haven't deeply looked at how to reimplement it to reduce complexity, hope you can try to do it. The symbols code already is complicated, I know there is intrinsic complexity here, but we really need to try to avoid making it even more complicaed. - Arnaldo > > dso__set_loaded(dso, map->type); > > @@ -1692,6 +1845,7 @@ restart: > continue; > > ret = dso__load_sym(dso, map, name, fd, filter, 0, > + &saved_shdrs, &saved_shdrs_count, > want_symtab); > close(fd); > > @@ -1713,14 +1867,19 @@ restart: > > /* > * If we wanted a full symtab but no image had one, > - * relax our requirements and repeat the search. > + * relax our requirements and repeat the search, > + * provided we saw some valid section headers: > */ > - if (ret <= 0 && want_symtab) { > + if (ret <= 0 && want_symtab && saved_shdrs != NULL) { > want_symtab = 0; > goto restart; > } > > free(name); > + > + if (saved_shdrs) > + free(saved_shdrs); > + > if (ret < 0 && strstr(dso->name, " (deleted)") != NULL) > return 0; > return ret; > @@ -1989,7 +2148,8 @@ int dso__load_vmlinux(struct dso *dso, struct map *map, > > dso__set_long_name(dso, (char *)vmlinux); > dso__set_loaded(dso, map->type); > - err = dso__load_sym(dso, map, symfs_vmlinux, fd, filter, 0, 0); > + err = dso__load_sym(dso, map, symfs_vmlinux, fd, filter, 0, > + NULL, NULL, 0); > close(fd); > > if (err > 0) > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>