lkml.org 
[lkml]   [2010]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH v4] perf symbols: fix symbol offset breakage with separated debug
    From
    Oops... it looks like I accidentally exported this patch with no
    signed-of-by line.

    Will repost shortly.

    Apologies
    ---Dave

    On Mon, Dec 6, 2010 at 5:25 PM, Dave Martin <dave.martin@linaro.org> wrote:
    > v4: remove assert() calls
    >
    >  * Simply removed the assert() to validate section indices
    >    returned by libelf: libelf should validate the ELF file
    >    anyway.
    >
    >  * Replaced the assert() on arguments to dso__load_sym
    >    with a run-time check.
    >
    > v3: Fixed a minor error in v2 where a check was erronueously
    > done twice.
    >
    > Original description follows:
    >
    > The new 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.
    >
    > The image search loop is also tidied up to fix a minor bug
    > which would perform the same image load attempt more than
    > once in some situations.
    >
    > For non-separated images, the headers should get loaded from
    > the same image as the symbols, in the usual way.
    > ---
    > KernelVersion: 2.6.37-rc3
    >
    >  tools/perf/util/symbol.c |  188 +++++++++++++++++++++++++++++++++++++++++++---
    >  1 files changed, 176 insertions(+), 12 deletions(-)
    >
    > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
    > index b39f499..7d8fd34 100644
    > --- a/tools/perf/util/symbol.c
    > +++ b/tools/perf/util/symbol.c
    > @@ -992,8 +992,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);
    > +       if (!shdrs)
    > +               return NULL;
    > +
    > +       for (i = 0; i < ehdr->e_shnum; i++)
    > +               shdrs[i].sh_type = SHT_NULL;
    > +
    > +       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.
    > + */
    > +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.
    > + *
    > + * 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 *self, 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 = self->kernel ? map__kmap(map) : NULL;
    > @@ -1012,6 +1109,17 @@ static int dso__load_sym(struct dso *self, 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;
    > +       }
    > +
    >        elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
    >        if (elf == NULL) {
    >                pr_debug("%s: cannot read %s ELF file.\n", __func__, name);
    > @@ -1035,6 +1143,34 @@ static int dso__load_sym(struct dso *self, 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)
    > @@ -1167,12 +1303,25 @@ static int dso__load_sym(struct dso *self, 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: %#Lx "
    >                                  "sh_addr: %#Lx sh_offset: %#Lx\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;
    >                }
    >                /*
    >                 * We need to figure out if the object was created from C++ sources
    > @@ -1409,6 +1558,8 @@ int dso__load(struct dso *self, 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;
    >
    >        dso__set_loaded(self, map->type);
    >
    > @@ -1439,13 +1590,13 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
    >         * On the first pass, only load images if they have a full symtab.
    >         * Failing that, do a second pass where we accept .dynsym also
    >         */
    > -       for (self->origin = DSO__ORIG_BUILD_ID_CACHE, want_symtab = 1;
    > -            self->origin != DSO__ORIG_NOT_FOUND;
    > -            self->origin++) {
    > +       self->origin = DSO__ORIG_BUILD_ID_CACHE;
    > +       want_symtab = 1;
    > +       while (1) {
    >                switch (self->origin) {
    >                case DSO__ORIG_BUILD_ID_CACHE:
    >                        if (dso__build_id_filename(self, name, size) == NULL)
    > -                               continue;
    > +                               goto continue_next;
    >                        break;
    >                case DSO__ORIG_FEDORA:
    >                        snprintf(name, size, "/usr/lib/debug%s.debug",
    > @@ -1459,7 +1610,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
    >                        char build_id_hex[BUILD_ID_SIZE * 2 + 1];
    >
    >                        if (!self->has_build_id)
    > -                               continue;
    > +                               goto continue_next;
    >
    >                        build_id__sprintf(self->build_id,
    >                                          sizeof(self->build_id),
    > @@ -1483,21 +1634,24 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
    >                default:
    >                        /*
    >                         * 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,
    > +                        * providing we saw some valid section headers:
    >                         */
    > -                       if (want_symtab) {
    > +                       if (want_symtab && saved_shdrs != NULL) {
    >                                want_symtab = 0;
    >                                self->origin = DSO__ORIG_BUILD_ID_CACHE;
    > -                       } else
    >                                continue;
    > +                       } else
    > +                               goto done;
    >                }
    >
    >                /* Name is now the name of the next image to try */
    >                fd = open(name, O_RDONLY);
    >                if (fd < 0)
    > -                       continue;
    > +                       goto continue_next;
    >
    >                ret = dso__load_sym(self, map, name, fd, filter, 0,
    > +                                   &saved_shdrs, &saved_shdrs_count,
    >                                    want_symtab);
    >                close(fd);
    >
    > @@ -1506,7 +1660,7 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
    >                 * info!?!?
    >                 */
    >                if (!ret)
    > -                       continue;
    > +                       goto continue_next;
    >
    >                if (ret > 0) {
    >                        int nr_plt = dso__synthesize_plt_symbols(self, map, filter);
    > @@ -1514,9 +1668,18 @@ int dso__load(struct dso *self, struct map *map, symbol_filter_t filter)
    >                                ret += nr_plt;
    >                        break;
    >                }
    > +
    > +continue_next:
    > +               self->origin++;
    > +               continue;
    >        }
    >
    > +done:
    >        free(name);
    > +
    > +       if (saved_shdrs)
    > +               free(saved_shdrs);
    > +
    >        if (ret < 0 && strstr(self->name, " (deleted)") != NULL)
    >                return 0;
    >        return ret;
    > @@ -1782,7 +1945,8 @@ static int dso__load_vmlinux(struct dso *self, struct map *map,
    >                return -1;
    >
    >        dso__set_loaded(self, map->type);
    > -       err = dso__load_sym(self, map, vmlinux, fd, filter, 0, 0);
    > +       err = dso__load_sym(self, map, vmlinux, fd, filter, 0,
    > +                           NULL, NULL, 0);
    >        close(fd);
    >
    >        if (err > 0)
    > --
    > 1.7.1
    >
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2010-12-06 18:53    [W:0.049 / U:1.164 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site