Re: [PATCH 2/3] btf_encoder: Change functions check due to broken dwarf

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Oct 27, 2020 at 04:20:10PM -0700, Andrii Nakryiko wrote:
> On Mon, Oct 26, 2020 at 5:07 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We need to generate just single BTF instance for the
> > function, while DWARF data contains multiple instances
> > of DW_TAG_subprogram tag.
> >
> > Unfortunately we can no longer rely on DW_AT_declaration
> > tag (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060)
> >
> > Instead we apply following checks:
> >   - argument names are defined for the function
> >   - there's symbol and address defined for the function
> >   - function is generated only once
> >
> > They might be slightly superfluous together, but it's
> > better to be ready for another DWARF mishap.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  btf_encoder.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  elf_symtab.h  |   8 ++++
> >  2 files changed, 109 insertions(+), 1 deletion(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index 2dd26c904039..99b9abe36993 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -26,6 +26,62 @@
> >   */
> >  #define KSYM_NAME_LEN 128
> >
> > +struct elf_function {
> > +       const char *name;
> > +       bool generated;
> > +};
> > +
> > +static struct elf_function *functions;
> > +static int functions_alloc;
> > +static int functions_cnt;
> > +
> > +static int functions_cmp(const void *_a, const void *_b)
> > +{
> > +       const struct elf_function *a = _a;
> > +       const struct elf_function *b = _b;
> > +
> > +       return strcmp(a->name, b->name);
> > +}
> > +
> > +static void delete_functions(void)
> > +{
> > +       free(functions);
> > +       functions_alloc = functions_cnt = 0;
> > +}
> > +
> > +static int config_function(struct btf_elf *btfe, GElf_Sym *sym)
> > +{
> > +       if (!elf_sym__is_function(sym))
> > +               return 0;
> > +       if (!elf_sym__value(sym))
> > +               return 0;
> > +
> > +       if (functions_cnt == functions_alloc) {
> > +               functions_alloc += 5000;
> 
> maybe just do a conventional exponential size increase? Not
> necessarily * 2, could be (* 3 / 2) or (* 4 / 3), libbpf uses such
> approach.

ok, will change

> 
> > +               functions = realloc(functions, functions_alloc * sizeof(*functions));
> > +               if (!functions)
> > +                       return -1;
> > +       }
> > +
> > +       functions[functions_cnt].name = elf_sym__name(sym, btfe->symtab);
> > +       functions_cnt++;
> > +       return 0;
> > +}
> > +
> 
> [...]
> 
> > diff --git a/elf_symtab.h b/elf_symtab.h
> > index 359add69c8ab..094ec4683d01 100644
> > --- a/elf_symtab.h
> > +++ b/elf_symtab.h
> > @@ -63,6 +63,14 @@ static inline uint64_t elf_sym__value(const GElf_Sym *sym)
> >         return sym->st_value;
> >  }
> >
> > +static inline int elf_sym__is_function(const GElf_Sym *sym)
> > +{
> > +       return (elf_sym__type(sym) == STT_FUNC ||
> > +               elf_sym__type(sym) == STT_GNU_IFUNC) &&
> 
> Why do we need to collect STT_GNU_IFUNC? That is some PLT special
> magic, does the kernel use that? Even if it does, are we even able to
> attach to that? Could that remove some of the assembly functions?

I missed that when I copied that function from perf ;-) I'll check

jirka

> 
> > +               sym->st_name != 0 &&
> > +               sym->st_shndx != SHN_UNDEF;
> > +}
> > +
> >  static inline bool elf_sym__is_local_function(const GElf_Sym *sym)
> >  {
> >         return elf_sym__type(sym) == STT_FUNC &&
> > --
> > 2.26.2
> >
>