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
