| Message ID | 20220404102908.14688-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Support Debian in resolve_full_path() | expand |
On Mon, Apr 4, 2022 at 3:29 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > --- > 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__) > + search_paths[(*n)++] = "/lib/x86_64-linux-gnu"; > +#elif defined(__i386__) > + search_paths[(*n)++] = "/lib/i386-linux-gnu"; > +#elif defined(__s390x__) > + search_paths[(*n)++] = "/lib/s390x-linux-gnu"; > +#elif defined(__s390__) > + search_paths[(*n)++] = "/lib/s390-linux-gnu"; > +#elif defined(__arm__) > +#if defined(__SOFTFP__) > + search_paths[(*n)++] = "/lib/arm-linux-gnueabi"; > +#else > + search_paths[(*n)++] = "/lib/arm-linux-gnueabihf"; > +#endif /* defined(__SOFTFP__) */ > +#elif defined(__aarch64__) > + search_paths[(*n)++] = "/lib/aarch64-linux-gnu"; > +#elif defined(__mips__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > +#if _MIPS_SZLONG == 64 > + search_paths[(*n)++] = "/lib/mips64el-linux-gnuabi64"; > +#elif _MIPS_SZLONG == 32 > + search_paths[(*n)++] = "/lib/mipsel-linux-gnu"; > +#endif > +#elif defined(__powerpc__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > + search_paths[(*n)++] = "/lib/powerpc64le-linux-gnu"; > +#elif defined(__sparc__) > + search_paths[(*n)++] = "/lib/sparc64-linux-gnu"; > +#elif defined(__riscv) && __riscv_xlen == 64 > + search_paths[(*n)++] = "/lib/riscv64-linux-gnu"; > +#endif > +} > + that's pretty comprehensive :) But let's make this function return const char * instead, with NULL for unknown/unsupported architectures. This will make the below code simpler.... > /* Get full path to program/shared library. */ > static int resolve_full_path(const char *file, char *result, size_t result_sz) > { > - const char *search_paths[2]; > - int i; > + const char *search_paths[3]; > + int i, n = 0; > instead of counting, we can just NULL-initialize search_paths and teach the loop below to ignore NULL entries? > if (strstr(file, ".so")) { > - search_paths[0] = getenv("LD_LIBRARY_PATH"); > - search_paths[1] = "/usr/lib64:/usr/lib"; > + search_paths[n++] = getenv("LD_LIBRARY_PATH"); > + search_paths[n++] = "/usr/lib64:/usr/lib"; > + add_debian_library_paths(search_paths, &n); so you'll just have search_paths[2] = arch_specific_lib_paths(); > } else { > - search_paths[0] = getenv("PATH"); > - search_paths[1] = "/usr/bin:/usr/sbin"; > + search_paths[n++] = getenv("PATH"); > + search_paths[n++] = "/usr/bin:/usr/sbin"; > } > > - for (i = 0; i < ARRAY_SIZE(search_paths); i++) { > + for (i = 0; i < n; i++) { > const char *s; > > if (!search_paths[i]) oh, actually we already do ignore NULLs, it seems? > -- > 2.35.1 >
On Mon, Apr 4, 2022 at 3:29 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > --- > 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__) > + search_paths[(*n)++] = "/lib/x86_64-linux-gnu"; > +#elif defined(__i386__) > + search_paths[(*n)++] = "/lib/i386-linux-gnu"; > +#elif defined(__s390x__) > + search_paths[(*n)++] = "/lib/s390x-linux-gnu"; > +#elif defined(__s390__) > + search_paths[(*n)++] = "/lib/s390-linux-gnu"; > +#elif defined(__arm__) > +#if defined(__SOFTFP__) > + search_paths[(*n)++] = "/lib/arm-linux-gnueabi"; > +#else > + search_paths[(*n)++] = "/lib/arm-linux-gnueabihf"; > +#endif /* defined(__SOFTFP__) */ > +#elif defined(__aarch64__) > + search_paths[(*n)++] = "/lib/aarch64-linux-gnu"; > +#elif defined(__mips__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > +#if _MIPS_SZLONG == 64 > + search_paths[(*n)++] = "/lib/mips64el-linux-gnuabi64"; > +#elif _MIPS_SZLONG == 32 > + search_paths[(*n)++] = "/lib/mipsel-linux-gnu"; > +#endif > +#elif defined(__powerpc__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) > + search_paths[(*n)++] = "/lib/powerpc64le-linux-gnu"; > +#elif defined(__sparc__) > + search_paths[(*n)++] = "/lib/sparc64-linux-gnu"; > +#elif defined(__riscv) && __riscv_xlen == 64 > + search_paths[(*n)++] = "/lib/riscv64-linux-gnu"; > +#endif > +} > + > /* Get full path to program/shared library. */ > static int resolve_full_path(const char *file, char *result, size_t result_sz) > { > - const char *search_paths[2]; > - int i; > + const char *search_paths[3]; > + int i, n = 0; > > if (strstr(file, ".so")) { > - search_paths[0] = getenv("LD_LIBRARY_PATH"); > - search_paths[1] = "/usr/lib64:/usr/lib"; > + search_paths[n++] = getenv("LD_LIBRARY_PATH"); > + search_paths[n++] = "/usr/lib64:/usr/lib"; > + add_debian_library_paths(search_paths, &n); > } else { > - search_paths[0] = getenv("PATH"); > - search_paths[1] = "/usr/bin:/usr/sbin"; > + search_paths[n++] = getenv("PATH"); > + search_paths[n++] = "/usr/bin:/usr/sbin"; > } > > - for (i = 0; i < ARRAY_SIZE(search_paths); i++) { > + for (i = 0; i < n; i++) { > const char *s; > > if (!search_paths[i]) > -- > 2.35.1 >
On Mon, 2022-04-04 at 14:58 -0700, Andrii Nakryiko wrote: > On Mon, Apr 4, 2022 at 3:29 AM Ilya Leoshkevich <iii@linux.ibm.com> > 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@linux.ibm.com> > > --- > > 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.
On Mon, Apr 4, 2022 at 3:22 PM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > > 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@linux.ibm.com> > > > --- > > > 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.
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__) + search_paths[(*n)++] = "/lib/x86_64-linux-gnu"; +#elif defined(__i386__) + search_paths[(*n)++] = "/lib/i386-linux-gnu"; +#elif defined(__s390x__) + search_paths[(*n)++] = "/lib/s390x-linux-gnu"; +#elif defined(__s390__) + search_paths[(*n)++] = "/lib/s390-linux-gnu"; +#elif defined(__arm__) +#if defined(__SOFTFP__) + search_paths[(*n)++] = "/lib/arm-linux-gnueabi"; +#else + search_paths[(*n)++] = "/lib/arm-linux-gnueabihf"; +#endif /* defined(__SOFTFP__) */ +#elif defined(__aarch64__) + search_paths[(*n)++] = "/lib/aarch64-linux-gnu"; +#elif defined(__mips__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) +#if _MIPS_SZLONG == 64 + search_paths[(*n)++] = "/lib/mips64el-linux-gnuabi64"; +#elif _MIPS_SZLONG == 32 + search_paths[(*n)++] = "/lib/mipsel-linux-gnu"; +#endif +#elif defined(__powerpc__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__) + search_paths[(*n)++] = "/lib/powerpc64le-linux-gnu"; +#elif defined(__sparc__) + search_paths[(*n)++] = "/lib/sparc64-linux-gnu"; +#elif defined(__riscv) && __riscv_xlen == 64 + search_paths[(*n)++] = "/lib/riscv64-linux-gnu"; +#endif +} + /* Get full path to program/shared library. */ static int resolve_full_path(const char *file, char *result, size_t result_sz) { - const char *search_paths[2]; - int i; + const char *search_paths[3]; + int i, n = 0; if (strstr(file, ".so")) { - search_paths[0] = getenv("LD_LIBRARY_PATH"); - search_paths[1] = "/usr/lib64:/usr/lib"; + search_paths[n++] = getenv("LD_LIBRARY_PATH"); + search_paths[n++] = "/usr/lib64:/usr/lib"; + add_debian_library_paths(search_paths, &n); } else { - search_paths[0] = getenv("PATH"); - search_paths[1] = "/usr/bin:/usr/sbin"; + search_paths[n++] = getenv("PATH"); + search_paths[n++] = "/usr/bin:/usr/sbin"; } - for (i = 0; i < ARRAY_SIZE(search_paths); i++) { + for (i = 0; i < n; i++) { const char *s; if (!search_paths[i])
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@linux.ibm.com> --- tools/lib/bpf/libbpf.c | 54 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 7 deletions(-)