On Wed, Feb 10, 2021 at 11:11 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> On Wed, Feb 10, 2021 at 10:20:20AM -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 10, 2021 at 5:26 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 09, 2021 at 02:00:29PM -0800, Andrii Nakryiko wrote:
> > >
> > > SNIP
> > >
> > > > > > > I'm still trying to build the kernel.. however ;-)
> > > > > > >
> > > > > > > patch below adds the ftrace check only for static functions
> > > > > > > and lets the externa go through.. but as you said, in this
> > > > > > > case we'll need to figure out the 'notrace' and other checks
> > > > > > > ftrace is doing
> > > > > > >
> > > > > > > jirka
> > > > > > >
> > > > > > >
> > > > > > > ---
> > > > > > > diff --git a/btf_encoder.c b/btf_encoder.c
> > > > > > > index b124ec20a689..4d147406cfa5 100644
> > > > > > > --- a/btf_encoder.c
> > > > > > > +++ b/btf_encoder.c
> > > > > > > @@ -734,7 +734,7 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
> > > > > > > continue;
> > > > > > > if (!has_arg_names(cu, &fn->proto))
> > > > > > > continue;
> > > > > > > - if (functions_cnt) {
> > > > > > > + if (!fn->external && functions_cnt) {
> > > > > >
> > > > > > I wouldn't trust DWARF, honestly. Wouldn't checking GLOBAL vs LOCAL
> > > > > > FUNC ELF symbol be more reliable?
> > > > >
> > > > > that'd mean extra bsearch on each processed function,
> > > > > on the ther hand, we'are already slow ;-) I'll check
> > > > > how big the slowdown would be
> > > > >
> > > >
> > > > We currently record addresses and do binary search. Now we need to
> > > > record address + size and still do binary search with a slightly
> > > > different semantics (find closest entry >= addr). Then just check that
> > > > it overlaps, taking into account the length of the function code. It
> > > > shouldn't result in a noticeable slowdown. Might be actually faster,
> > > > because we might avoid callback function call costs.
> > >
> > > I'm still not sure how to handle the external check for function via elf,
> >
> > I might be missing something, but don't all functions have
> > corresponding ELF symbols? And then symbol can have LOCAL or GLOBAL
> > type. LOCALs are supposed to be not visible outside respective CUs (so
> > correspond to static functions), while GLOBALs are extern-able funcs.
> > So if func's symbol is GLOBAL, it should be ok to assume it's
> > attachable (not inlined, at least).
>
> sure I know where the info is, I was just hesitating to add another bsearch
>
> anyway, I checked the BTF data before and after this change (the dwarf check above)
> and it looks like there are many global functions that are not attachable
>
> attaching diff of BTF functions below - before (.old) and after (.new) adding
> all external functions.. all added (+) functions are external and do not have
> ftrace address - I verified some of them in disassembly and they are not attachable
>
> I think we should check ftrace addresses also for global functions
>
> I'm of course assuming BTF should have only attachable functions
Oh, ok, I didn't realize so many functions are not attachable. Yeah,
let's not do anything special for GLOBALs, given we are fixing the
ftrace addr search anyway.
>
> jirka
>
>
> ---
> --- /tmp/pahole.test.PbD/funcs-vmlinux-x86-gcc-1.old 2021-02-10 19:51:44.727865204 +0100
> +++ /tmp/pahole.test.PbD/funcs-vmlinux-x86-gcc-1.new 2021-02-10 19:51:44.836865827 +0100
> @@ -1662,6 +1662,7 @@
[...]
