Re: [RFC/PATCH 3/3] btf_encoder: Func generation fix

[Jiri Olsa <jolsa@redhat.com>]

On Thu, Nov 12, 2020 at 11:54:41AM -0800, Andrii Nakryiko wrote:

SNIP

> > @@ -624,32 +644,46 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> >                 has_index_type = true;
> >         }
> >
> > -       cu__for_each_function(cu, core_id, fn) {
> > -               /*
> > -                * The functions_cnt != 0 means we parsed all necessary
> > -                * kernel symbols and we are using ftrace location filter
> > -                * for functions. If it's not available keep the current
> > -                * dwarf declaration check.
> > -                */
> > -               if (functions_cnt) {
> > +       /*
> > +        * The functions_cnt != 0 means we parsed all necessary
> > +        * kernel symbols and we are using ftrace location filter
> > +        * for functions. If it's not available keep the current
> > +        * dwarf declaration check.
> > +        */
> > +       if (functions_cnt) {
> > +               cu__for_each_function(cu, core_id, fn) {
> > +                       struct elf_function *p;
> > +                       struct elf_function key = { .name = function__name(fn, cu) };
> > +                       int args_cnt = 0;
> > +
> >                         /*
> > -                        * We check following conditions:
> > -                        *   - argument names are defined
> > -                        *   - there's symbol and address defined for the function
> > -                        *   - function address belongs to ftrace locations
> > -                        *   - function is generated only once
> > +                        * Collect functions that match ftrace filter
> > +                        * and pick the one with proper argument names.
> > +                        * The BTF generation happens at the end in
> > +                        * btf_encoder__encode function.
> >                          */
> > -                       if (!has_arg_names(cu, &fn->proto))
> > +                       p = bsearch(&key, functions, functions_cnt,
> > +                                   sizeof(functions[0]), functions_cmp);
> > +                       if (!p)
> >                                 continue;
> > -                       if (!should_generate_function(btfe, function__name(fn, cu)))
> > +
> > +                       if (!has_arg_names(cu, &fn->proto, &args_cnt))
> 
> So I can't unfortunately reproduce that GCC bug with DWARF info. What
> was exactly the symptom? Maybe you can also share readelf -wi dump for
> your problematic vmlinux?

hum, can't see -wi working for readelf, however I placed my vmlinux
in here:
  http://people.redhat.com/~jolsa/vmlinux.gz

the symptom is that resolve_btfids will fail kernel build:

  BTFIDS  vmlinux
FAILED unresolved symbol vfs_getattr

because BTF data contains multiple FUNC records for same function

and the problem is that declaration tag itself is missing:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060
so pahole won't skip them

the first workaround was to workaround that and go for function
records that have code address attached, but that's buggy as well:
  https://bugzilla.redhat.com/show_bug.cgi?id=1890107

then after some discussions we ended up using ftrace addresses
as filter for what we want to display.. plus we got static functions
as well

however for this way we failed to get proper arguments ;-)

> 
> The reason I'm asking is because I wonder if we should still ignore
> functions if fn->declaration is set. E.g., for the issue we
> investigated yesterday, the function with no arguments has declaration
> set to 1, so just ignoring it would solve the problem. I'm wondering
> if it's enough to do just that instead of doing this whole delayed
> function collection/processing.
> 
> Also, I'd imagine the only expected cases where we can override  the
> function (args_cnt > p->args_cnt) would be if p->args_cnt == 0, no?

I don't know, because originally I'd expect that we would not see
function record with zero args when it actualy has some

> All other cases are either newly discovered "bogusness" of DWARF (and
> would be good to know about this) or it's a name collision for
> functions. Basically, before we go all the way to rework this again,
> let's see if just skipping declarations would be enough?

so there's actualy new developement today in:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97060#c14

gcc might actualy get fixed, so I think we could go back to
using declaration tag and use ftrace as additional filter..
at least this exercise gave us static functions ;-)

however we still have fedora with disabled disabled CONFIG_DEBUG_INFO_BTF
and will need to wait for that fix to enable that back

> 
> >                                 continue;
> > -               } else {
> > +
> > +                       if (!p->fn || args_cnt > p->args_cnt) {
> > +                               p->fn = fn;
> > +                               p->cu = cu;
> > +                               p->args_cnt = args_cnt;
> > +                               p->type_id_off = type_id_off;
> > +                       }
> > +               }
> > +       } else {
> > +               cu__for_each_function(cu, core_id, fn) {
> >                         if (fn->declaration || !fn->external)
> >                                 continue;
> > +                       if (generate_func(btfe, cu, fn, type_id_off))
> > +                               goto out;
> >                 }
> 
> I'm trending towards disliking this completely different fallback
> mechanism. It saved bpf-next accidentally, but otherwise obscured the
> issue and generally makes testing pahole with artificial binary BTFs
> (from test programs) harder. How about we unify approaches, but just
> use mcount symbols opportunistically, as an additional filter, if it's
> available?

ok

> 
> With that, testing that we still handle functions with duplicate names
> properly would be trivial (which I suspect we don't and we'll just
> keep the one with more args now, right?) And it makes static functions
> available for non-vmlinux binaries automatically (might be good or
> bad, but still...).

if we keep just the ftrace filter and rely on declaration tag,
the args checking will go away right?

jirka