Re: [PATCH bpf-next] selftests/bpf: Fix test_attach_probe for powerpc uprobes

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

On Tue, Mar 2, 2021 at 3:14 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Mon, Mar 01, 2021 at 04:34:24PM -0800, Andrii Nakryiko wrote:
> > On Mon, Mar 1, 2021 at 11:11 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > When testing uprobes we the test gets GEP (Global Entry Point)
> > > address from kallsyms, but then the function is called locally
> > > so the uprobe is not triggered.
> > >
> > > Fixing this by adjusting the address to LEP (Local Entry Point)
> > > for powerpc arch.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  .../selftests/bpf/prog_tests/attach_probe.c    | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > index a0ee87c8e1ea..c3cfb48d3ed0 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> > > @@ -2,6 +2,22 @@
> > >  #include <test_progs.h>
> > >  #include "test_attach_probe.skel.h"
> > >
> > > +#if defined(__powerpc64__)
> > > +/*
> > > + * We get the GEP (Global Entry Point) address from kallsyms,
> > > + * but then the function is called locally, so we need to adjust
> > > + * the address to get LEP (Local Entry Point).
> > > + */
> > > +#define LEP_OFFSET 8
> > > +
> > > +static ssize_t get_offset(ssize_t offset)
> >
> > if we mark this function __weak global, would it work as is? Would it
> > get an address of a global entry point? I know nothing about this GEP
> > vs LEP stuff, interesting :)
>
> you mean get_base_addr? it's already global
>
> all the calls to get_base_addr within the object are made
> to get_base_addr+0x8
>
> 00000000100350c0 <test_attach_probe>:
>     ...
>     100350e0:   59 fd ff 4b     bl      10034e38 <get_base_addr+0x8>
>     ...
>     100358a8:   91 f5 ff 4b     bl      10034e38 <get_base_addr+0x8>
>
>
> I'm following perf fix we had for similar issue:
>   7b6ff0bdbf4f perf probe ppc64le: Fixup function entry if using kallsyms lookup
>
> I'll get more info on that

My thinking was that if you mark the function as __weak, then the
compiler is not allowed to assume that the actual implementation of
that function will come from the same object (because it might be
replaced by the linker later), so it has to be pessimistic and use
global entry, no? Totally theoritizing here, of course.

>
> jirka
>
> >
> > > +{
> > > +       return offset + LEP_OFFSET;
> > > +}
> > > +#else
> > > +#define get_offset(offset) (offset)
> > > +#endif
> > > +
> > >  ssize_t get_base_addr() {
> > >         size_t start, offset;
> > >         char buf[256];
> > > @@ -36,7 +52,7 @@ void test_attach_probe(void)
> > >         if (CHECK(base_addr < 0, "get_base_addr",
> > >                   "failed to find base addr: %zd", base_addr))
> > >                 return;
> > > -       uprobe_offset = (size_t)&get_base_addr - base_addr;
> > > +       uprobe_offset = get_offset((size_t)&get_base_addr - base_addr);
> > >
> > >         skel = test_attach_probe__open_and_load();
> > >         if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
> > > --
> > > 2.29.2
> > >
> >
>