[PATCH v2] libbpf: Fix determine_ptr_size() guessing

[PATCH v2] libbpf: Fix determine_ptr_size() guessing

[Douglas RAILLARD]

From: Douglas Raillard <douglas.raillard@arm.com>

One strategy employed by libbpf to guess the pointer size is by finding
the size of "unsigned long" type. This is achieved by looking for a type
of with the expected name and checking its size.

Unfortunately, the C syntax is friendlier to humans than to computers
as there is some variety in how such a type can be named. Specifically,
gcc and clang do not use the same name in debug info.

Lookup all the names for such a type so that libbpf can hope to find the
information it wants.

Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
---
 tools/lib/bpf/btf.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

 CHANGELOG
	v2:
		* Added missing case for "long"

diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 1383e26c5d1f..ab92b3bc2724 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -489,8 +489,19 @@ static int determine_ptr_size(const struct btf *btf)
 		if (!name)
 			continue;
 
-		if (strcmp(name, "long int") == 0 ||
-		    strcmp(name, "long unsigned int") == 0) {
+		if (
+			strcmp(name, "long") == 0 ||
+			strcmp(name, "long int") == 0 ||
+			strcmp(name, "int long") == 0 ||
+			strcmp(name, "unsigned long") == 0 ||
+			strcmp(name, "long unsigned") == 0 ||
+			strcmp(name, "unsigned long int") == 0 ||
+			strcmp(name, "unsigned int long") == 0 ||
+			strcmp(name, "long unsigned int") == 0 ||
+			strcmp(name, "long int unsigned") == 0 ||
+			strcmp(name, "int unsigned long") == 0 ||
+			strcmp(name, "int long unsigned") == 0
+		) {
 			if (t->size != 4 && t->size != 8)
 				continue;
 			return t->size;
-- 
2.25.1

Re: [PATCH v2] libbpf: Fix determine_ptr_size() guessing

[Yonghong Song]

On 5/23/22 3:29 AM, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> One strategy employed by libbpf to guess the pointer size is by finding
> the size of "unsigned long" type. This is achieved by looking for a type
> of with the expected name and checking its size.
> 
> Unfortunately, the C syntax is friendlier to humans than to computers
> as there is some variety in how such a type can be named. Specifically,
> gcc and clang do not use the same name in debug info.
> 
> Lookup all the names for such a type so that libbpf can hope to find the
> information it wants.
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>

Acked-by: Yonghong Song <yhs@fb.com>

Re: [PATCH v2] libbpf: Fix determine_ptr_size() guessing

[Daniel Borkmann]

On 5/23/22 12:29 PM, Douglas RAILLARD wrote:
> From: Douglas Raillard <douglas.raillard@arm.com>
> 
> One strategy employed by libbpf to guess the pointer size is by finding
> the size of "unsigned long" type. This is achieved by looking for a type
> of with the expected name and checking its size.
> 
> Unfortunately, the C syntax is friendlier to humans than to computers
> as there is some variety in how such a type can be named. Specifically,
> gcc and clang do not use the same name in debug info.

Could you elaborate for the commit msg what both emit differently?

> Lookup all the names for such a type so that libbpf can hope to find the
> information it wants.
> 
> Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> ---
>   tools/lib/bpf/btf.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
>   CHANGELOG
> 	v2:
> 		* Added missing case for "long"
> 
> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> index 1383e26c5d1f..ab92b3bc2724 100644
> --- a/tools/lib/bpf/btf.c
> +++ b/tools/lib/bpf/btf.c
> @@ -489,8 +489,19 @@ static int determine_ptr_size(const struct btf *btf)
>   		if (!name)
>   			continue;
>   
> -		if (strcmp(name, "long int") == 0 ||
> -		    strcmp(name, "long unsigned int") == 0) {
> +		if (
> +			strcmp(name, "long") == 0 ||
> +			strcmp(name, "long int") == 0 ||
> +			strcmp(name, "int long") == 0 ||
> +			strcmp(name, "unsigned long") == 0 ||
> +			strcmp(name, "long unsigned") == 0 ||
> +			strcmp(name, "unsigned long int") == 0 ||
> +			strcmp(name, "unsigned int long") == 0 ||
> +			strcmp(name, "long unsigned int") == 0 ||
> +			strcmp(name, "long int unsigned") == 0 ||
> +			strcmp(name, "int unsigned long") == 0 ||
> +			strcmp(name, "int long unsigned") == 0
> +		) {

I was wondering whether strstr(3) or regexec(3) would be better, but then it's
probably not worth it and having the different combinations spelled out is
probably still better. Pls make sure though to stick to kernel coding convention
(similar alignment around strcmp() as the lines you remove).

>   			if (t->size != 4 && t->size != 8)
>   				continue;
>   			return t->size;
> 

Thanks,
Daniel

Re: [PATCH v2] libbpf: Fix determine_ptr_size() guessing

[Andrii Nakryiko]

On Mon, May 23, 2022 at 2:00 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 5/23/22 12:29 PM, Douglas RAILLARD wrote:
> > From: Douglas Raillard <douglas.raillard@arm.com>
> >
> > One strategy employed by libbpf to guess the pointer size is by finding
> > the size of "unsigned long" type. This is achieved by looking for a type
> > of with the expected name and checking its size.
> >
> > Unfortunately, the C syntax is friendlier to humans than to computers
> > as there is some variety in how such a type can be named. Specifically,
> > gcc and clang do not use the same name in debug info.
>
> Could you elaborate for the commit msg what both emit differently?
>
> > Lookup all the names for such a type so that libbpf can hope to find the
> > information it wants.
> >
> > Signed-off-by: Douglas Raillard <douglas.raillard@arm.com>
> > ---
> >   tools/lib/bpf/btf.c | 15 +++++++++++++--
> >   1 file changed, 13 insertions(+), 2 deletions(-)
> >
> >   CHANGELOG
> >       v2:
> >               * Added missing case for "long"
> >
> > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
> > index 1383e26c5d1f..ab92b3bc2724 100644
> > --- a/tools/lib/bpf/btf.c
> > +++ b/tools/lib/bpf/btf.c
> > @@ -489,8 +489,19 @@ static int determine_ptr_size(const struct btf *btf)
> >               if (!name)
> >                       continue;
> >
> > -             if (strcmp(name, "long int") == 0 ||
> > -                 strcmp(name, "long unsigned int") == 0) {
> > +             if (
> > +                     strcmp(name, "long") == 0 ||
> > +                     strcmp(name, "long int") == 0 ||
> > +                     strcmp(name, "int long") == 0 ||
> > +                     strcmp(name, "unsigned long") == 0 ||
> > +                     strcmp(name, "long unsigned") == 0 ||
> > +                     strcmp(name, "unsigned long int") == 0 ||
> > +                     strcmp(name, "unsigned int long") == 0 ||
> > +                     strcmp(name, "long unsigned int") == 0 ||
> > +                     strcmp(name, "long int unsigned") == 0 ||
> > +                     strcmp(name, "int unsigned long") == 0 ||
> > +                     strcmp(name, "int long unsigned") == 0
> > +             ) {
>
> I was wondering whether strstr(3) or regexec(3) would be better, but then it's

regexec() seems like an overkill, but strstr() won't work because
we'll mistakingly find "long long". Splitting by space and sorting
also feels like going a bit too far. So I guess let's stick to this
exhaustive comparison approach.

But Douglas, can you please a table instead of writing out all those strcmp():

const char *long_aliases[] = {
    "long",
    "long int",
    ...
}

for (i = 0; i < ARRAY_SIZE(long_aliases); i++) { ... }

?

> probably not worth it and having the different combinations spelled out is
> probably still better. Pls make sure though to stick to kernel coding convention
> (similar alignment around strcmp() as the lines you remove).
>
> >                       if (t->size != 4 && t->size != 8)
> >                               continue;
> >                       return t->size;
> >
>
> Thanks,
> Daniel