Re: [PATCH v2 03/19] gendwarfksyms: Add address matching

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Petr,

On Tue, Aug 27, 2024 at 12:40 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > +static inline u32 symbol_addr_hash(const struct symbol_addr *addr)
> > +{
> > +     return jhash(addr, sizeof(struct symbol_addr), 0);
>
> I would be careful and avoid including the padding between
> symbol_addr.section and symbol_addr.address in the hash calculation.

Good catch. I'll fix this in the next version.

> > +static int elf_for_each_symbol(int fd, elf_symbol_callback_t func, void *arg)
> > +{
> > +     size_t sym_size;
> > +     GElf_Shdr shdr_mem;
> > +     GElf_Shdr *shdr;
> > +     Elf_Data *xndx_data = NULL;
> > +     Elf_Scn *scn;
> > +     Elf *elf;
> > +
> > +     if (elf_version(EV_CURRENT) != EV_CURRENT) {
> > +             error("elf_version failed: %s", elf_errmsg(-1));
> > +             return -1;
> > +     }
> > +
> > +     elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> > +     if (!elf) {
> > +             error("elf_begin failed: %s", elf_errmsg(-1));
> > +             return -1;
> > +     }
> > +
> > +     sym_size = gelf_getclass(elf) == ELFCLASS32 ? sizeof(Elf32_Sym) :
> > +                                                   sizeof(Elf64_Sym);
> > +
> > +     scn = elf_nextscn(elf, NULL);
> > +
> > +     while (scn) {
> > +             shdr = gelf_getshdr(scn, &shdr_mem);
> > +
> > +             if (shdr && shdr->sh_type == SHT_SYMTAB_SHNDX) {
> > +                     xndx_data = elf_getdata(scn, NULL);
> > +                     break;
> > +             }
> > +
> > +             scn = elf_nextscn(elf, scn);
> > +     }
> > +
> > +     scn = elf_nextscn(elf, NULL);
> > +
> > +     while (scn) {
> > +             shdr = gelf_getshdr(scn, &shdr_mem);
> > +
> > +             if (shdr && shdr->sh_type == SHT_SYMTAB) {
> > +                     Elf_Data *data = elf_getdata(scn, NULL);
> > +                     unsigned int nsyms = data->d_size / sym_size;
>
> I think strictly speaking this should be:
> size_t nsyms = shdr->sh_size / shdr->sh_entsize;
> .. and the code could check that shdr->sh_entsize is same as what
> gelf_fsize(elf, ELF_T_SYM, 1, EV_CURRENT) returns.

Sure, I can change this. I'm not sure if there's a situation where the
current calculation wouldn't result in the exact same result though.

> > +                     unsigned int n;
> > +
> > +                     for (n = 0; n < nsyms; ++n) {
>
> The first symbol in the symbol table is always undefined, the loop can
> start from 1.

Ack.

> Alternatively, since elf_for_each_symbol() ends up in the entire series
> being used only with process_symbol() which skips symbols with the local
> binding, the function could be renamed to elf_for_each_global_symbol()
> and start the loop from shdr->sh_info.

Patch 15 ("Add support for declaration-only data structures") actually
also needs to process local symbols, so we can't skip them completely.

Sami





[Index of Archives]     [Linux&nblp;USB Development]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite Secrets]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux