On Mon, Apr 4, 2022 at 3:22 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> On Mon, 2022-04-04 at 14:58 -0700, Andrii Nakryiko wrote:
> > On Mon, Apr 4, 2022 at 3:29 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > wrote:
> > >
> > > attach_probe selftest fails on Debian-based distros with `failed to
> > > resolve full path for 'libc.so.6'`. The reason is that these
> > > distros
> > > embraced multiarch to the point where even for the "main"
> > > architecture
> > > they store libc in /lib/<triple>.
> > >
> > > This is configured in /etc/ld.so.conf and in theory it's possible
> > > to
> > > replicate the loader's parsing and processing logic in libbpf,
> > > however
> > > a much simpler solution is to just enumerate the known library
> > > paths.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > ---
> > > tools/lib/bpf/libbpf.c | 54 ++++++++++++++++++++++++++++++++++++--
> > > ----
> > > 1 file changed, 47 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 6d2be53e4ba9..4f616b11564f 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -10707,21 +10707,61 @@ static long elf_find_func_offset(const
> > > char *binary_path, const char *name)
> > > return ret;
> > > }
> > >
> > > +static void add_debian_library_paths(const char **search_paths,
> > > int *n)
> > > +{
> > > + /*
> > > + * Based on https://packages.debian.org/sid/libc6.
> > > + *
> > > + * Assume that the traced program is built for the same
> > > architecture
> > > + * as libbpf, which should cover the vast majority of
> > > cases.
> > > + */
> > > +#if defined(__x86_64__)
> >
> > can you please also drop defined() where possible, it looks cleaner
> > to me:
> >
> > #if __x86_64__
> >
> > vs
> >
> > #if defined(__x86_64__)
>
> The consensus in the existing kernel and tools code (including libbpf
> itself) seems to be to use #if defined() or #ifdef for such macros:
>
> $ git grep __x86_64__ | wc -l
> 306
>
> $ git grep __x86_64__ | grep -v \
> -e '#\s*ifdef __x86_64__' \
> -e 'defined\s*(__x86_64__)' \
> -e '#\s*ifndef __x86_64__' \
> -e '#\s*else' \
> -e '#\s*endif'
> arch/x86/Makefile: CHECKFLAGS += -D__x86_64__
> arch/x86/Makefile.um:CHECKFLAGS += -m64 -D__x86_64__
> tools/lib/bpf/libbpf.c:#if __x86_64__
> tools/testing/selftests/ipc/Makefile: CFLAGS := -DCONFIG_X86_64 -
> D__x86_64__
> tools/testing/selftests/rcutorture/bin/mkinitrd.sh:if echo -e "#if
> __x86_64__||__i386__||__i486__||__i586__||__i686__" \
> tools/testing/selftests/x86/mov_ss_trap.c:#if __x86_64__
>
> I think `#if __x86_64__` should work in most cases, but I'd rather
> stick with the existing style if you don't mind.
Yeah, that's fine. I felt like #if defined() is unnecessarily verbose,
#ifdef would be totally fine. But looking at latest version, I think
it's fine, mostly due to more linear structure, so it's all good.
