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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Tue, Nov 3, 2020 at 11:06 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Nov 03, 2020 at 10:58:58AM -0800, Andrii Nakryiko wrote:
> > On Mon, Nov 2, 2020 at 2:57 PM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Mon, Nov 02, 2020 at 10:59:08PM +0100, Jiri Olsa wrote:
> > > > On Sat, Oct 31, 2020 at 11:31:31PM +0100, Jiri Olsa 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
> > > > >
> > > > > Also because we want to follow kernel's ftrace traceable
> > > > > functions, this patchset is adding extra check that the
> > > > > function is one of the ftrace's functions.
> > > > >
> > > > > All ftrace functions addresses are stored in vmlinux
> > > > > binary within symbols:
> > > > >   __start_mcount_loc
> > > > >   __stop_mcount_loc
> > > >
> > > > hum, for some reason this does not pass through bpf internal
> > > > functions like bpf_iter_bpf_map.. I learned it hard way ;-)
> >
> > what's the exact name of the function that was missing?
> > bpf_iter_bpf_map doesn't exist. And if it's __init function, why does
> > it matter, it's not going to be even available at runtime, right?
> >
>
> bpf_map iter definition:
>
> DEFINE_BPF_ITER_FUNC(bpf_map, struct bpf_iter_meta *meta, struct bpf_map *map)
>
> goes to:
>
> #define DEFINE_BPF_ITER_FUNC(target, args...)                   \
>         extern int bpf_iter_ ## target(args);                   \
>         int __init bpf_iter_ ## target(args) { return 0; }
>
> that creates __init bpf_iter_bpf_map function that will make
> it into BTF where it's expected when opening iterator, but the
> code will be freed because it's __init function

hm... should we just drop __init there?

Yonghong, is __init strictly necessary, or was just an optimization to
save a tiny bit of space?

>
> there are few iteratos functions like that, and I was going to
> check if there's more
>
> >
> > > > will check
> > >
> > > so it gets filtered out because it's __init function
> > > I'll check if the fix below catches all internal functions,
> > > but I guess we should do something more robust
> > >
> > > jirka
> > >
> > >
> > > ---
> > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > index 0a378aa92142..3cd94280c35b 100644
> > > --- a/btf_encoder.c
> > > +++ b/btf_encoder.c
> > > @@ -143,7 +143,8 @@ static int filter_functions(struct btf_elf *btfe, struct mcount_symbols *ms)
> > >                 /* Do not enable .init section functions. */
> > >                 if (init_filter &&
> > >                     func->addr >= ms->init_begin &&
> > > -                   func->addr <  ms->init_end)
> > > +                   func->addr <  ms->init_end &&
> > > +                   strncmp("bpf_", func->name, 4))
> >
> > this looks like a very wrong way to do this? Can you please elaborate
> > on what's missing and why it shouldn't be missing?
>
> yes, it's just a hack, we should do something more
> robust as I mentioned above
>
> it just allowed me to use iterators finaly ;-)

sure, I get it, I was just trying to understand why there is such a
problem in the first place. Turns out we need FUNCs not just for
fentry/fexit and similar, but also for bpf_iter, which is an entirely
different use case (similar to raw_tp, but raw_tp is using typedef ->
func_proto approach).

So I don't know, we might as well just not do mcount checks?.. As an
alternative, but it's not great as well.

>
> jirka
>