On Fri, Nov 13, 2020 at 12:56:40PM -0800, Andrii Nakryiko wrote:
> On Fri, Nov 13, 2020 at 7:13 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > Current conditions for picking up function records break
> > BTF data on some gcc versions.
> >
> > Some function records can appear with no arguments but with
> > declaration tag set, so moving the 'fn->declaration' in front
> > of other checks.
> >
> > Then checking if argument names are present and finally checking
> > ftrace filter if it's present. If ftrace filter is not available,
> > using the external tag to filter out non external functions.
> >
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
>
> I tested locally, all seems to work fine. Left few suggestions below,
> but those could be done in follow ups (or argued to not be done).
>
> Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
>
> BTW, for some stats.
>
> BEFORE allowing static funcs:
>
> .BTF ELF section
> =======================================
> Data size: 4101624
> Header size: 24
> Types size: 2472836
> Strings size: 1628764
>
> BTF types
> =======================================
> Total 2472836 bytes (83310 types)
> Struct: 920436 bytes (10305 types)
> FuncProto: 638668 bytes (18869 types)
> Func: 308304 bytes (25692 types)
> Enum: 184308 bytes (2293 types)
> Ptr: 173484 bytes (14457 types)
> Array: 89064 bytes (3711 types)
> Union: 81552 bytes (1961 types)
> Const: 34368 bytes (2864 types)
> Typedef: 32124 bytes (2677 types)
> Var: 4688 bytes (293 types)
> Datasec: 3528 bytes (1 types)
> Fwd: 1656 bytes (138 types)
> Volatile: 360 bytes (30 types)
> Int: 272 bytes (17 types)
> Restrict: 24 bytes (2 types)
>
>
> AFTER allowing static funcs:
>
> .BTF ELF section
> =======================================
> Data size: 4930558
> Header size: 24
> Types size: 2914016
> Strings size: 2016518
>
> BTF types
> =======================================
> Total 2914016 bytes (108282 types)
> Struct: 920436 bytes (10305 types)
> FuncProto: 851528 bytes (24814 types)
> Func: 536664 bytes (44722 types)
> Enum: 184308 bytes (2293 types)
> Ptr: 173484 bytes (14457 types)
> Array: 89064 bytes (3711 types)
> Union: 81552 bytes (1961 types)
> Const: 34368 bytes (2864 types)
> Typedef: 32124 bytes (2677 types)
> Var: 4688 bytes (293 types)
> Datasec: 3528 bytes (1 types)
> Fwd: 1656 bytes (138 types)
> Volatile: 360 bytes (30 types)
> Int: 256 bytes (16 types)
nice, is this tool somewhere in the tree?
>
> So 25692 vs 44722 functions, but the increase in func_proto is smaller
> due to dedup. Good chunk is strings data for all those function and
> parameter names.
>
>
> > btf_encoder.c | 24 ++++++++++--------------
> > 1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index d531651b1e9e..de471bc754b1 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -612,25 +612,21 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > const char *name;
> >
> > /*
> > - * 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.
> > + * Skip functions that:
> > + * - are marked as declarations
> > + * - do not have full argument names
> > + * - are not in ftrace list (if it's available)
> > + * - are not external (in case ftrace filter is not available)
> > */
> > + if (fn->declaration)
> > + continue;
> > + if (!has_arg_names(cu, &fn->proto))
> > + continue;
> > if (functions_cnt) {
> > - /*
> > - * 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
> > - */
> > - if (!has_arg_names(cu, &fn->proto))
> > - continue;
> > if (!should_generate_function(btfe, function__name(fn, cu)))
>
> Seeing Arnaldo's confusion, I remember initially I was similarly
> confused. I think this p->generated = true should be moved out of
> should_generate_function() and done here explicitly. Let's turn
> should_generate_function() into find_allowed_function() or something,
> to encapsulate bsearch. Checking !p || p->generated could be done here
> explicitly.
ok, that should be more obvious, I'll send new version
>
> > continue;
> > } else {
> > - if (fn->declaration || !fn->external)
> > + if (!fn->external)
>
> Hm.. why didn't you drop this fallback? For non-vmlinux, do you think
> it's a problem to generate all FUNCs? Mostly theoretical question,
> though.
because it would probably allowed all static functions,
(ftrace data has only static functions that are traceable)
and who knows what a can of worms we'd open here ;-)
jirka
