diff mbox series

[bpf-next] bpftool: Fix handling enum64 in btf dump sorting

Message ID 20240901213040.766724-1-yatsenko@meta.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] bpftool: Fix handling enum64 in btf dump sorting | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
netdev/tree_selection success Clearly marked for bpf-next
netdev/apply success Patch already applied to bpf-next-0

Commit Message

Mykyta Yatsenko mykyta.yatsenko5@gmail.com Sept. 1, 2024, 9:30 p.m. UTC 
From: Mykyta Yatsenko <yatsenko@meta.com>

Wrong function is used to access the first enum64 element.
Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/bpf/bpftool/btf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Comments

Daniel Borkmann Sept. 2, 2024, 4:22 p.m. UTC | #1 
On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Wrong function is used to access the first enum64 element.
> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>   tools/bpf/bpftool/btf.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..b0f12c511bb3 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>   	const struct btf_type *t = btf__type_by_id(btf, index);
>   
>   	switch (btf_kind(t)) {
> -	case BTF_KIND_ENUM:
> -	case BTF_KIND_ENUM64: {
> +	case BTF_KIND_ENUM: {
>   		int name_off = t->name_off;
>   
>   		/* Use name of the first element for anonymous enums if allowed */
> -		if (!from_ref && !t->name_off && btf_vlen(t))
> +		if (!from_ref && !name_off && btf_vlen(t))
>   			name_off = btf_enum(t)->name_off;
>   
>   		return btf__name_by_offset(btf, name_off);
>   	}

Small nit, could we consolidate the logic into the below? Still somewhat nicer
than duplicating all of the rest.

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..aae6f5262c6a 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
                 int name_off = t->name_off;

                 /* Use name of the first element for anonymous enums if allowed */
-               if (!from_ref && !t->name_off && btf_vlen(t))
-                       name_off = btf_enum(t)->name_off;
+               if (!from_ref && !name_off && btf_vlen(t))
+                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
+                                  btf_enum64(t)->name_off :
+                                  btf_enum(t)->name_off;

                 return btf__name_by_offset(btf, name_off);
         }

Thanks,
Daniel
Mykyta Yatsenko Sept. 2, 2024, 4:52 p.m. UTC | #2 
On 02/09/2024 17:22, Daniel Borkmann wrote:
> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Wrong function is used to access the first enum64 element.
>> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   tools/bpf/bpftool/btf.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 6789c7a4d5ca..b0f12c511bb3 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const 
>> struct btf *btf, __u32 index, bool f
>>       const struct btf_type *t = btf__type_by_id(btf, index);
>>         switch (btf_kind(t)) {
>> -    case BTF_KIND_ENUM:
>> -    case BTF_KIND_ENUM64: {
>> +    case BTF_KIND_ENUM: {
>>           int name_off = t->name_off;
>>             /* Use name of the first element for anonymous enums if 
>> allowed */
>> -        if (!from_ref && !t->name_off && btf_vlen(t))
>> +        if (!from_ref && !name_off && btf_vlen(t))
>>               name_off = btf_enum(t)->name_off;
>>             return btf__name_by_offset(btf, name_off);
>>       }
>
> Small nit, could we consolidate the logic into the below? Still 
> somewhat nicer
> than duplicating all of the rest.
Thanks for the suggestion, this makes sense,  I will apply it.
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..aae6f5262c6a 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const 
> struct btf *btf, __u32 index, bool f
>                 int name_off = t->name_off;
>
>                 /* Use name of the first element for anonymous enums 
> if allowed */
> -               if (!from_ref && !t->name_off && btf_vlen(t))
> -                       name_off = btf_enum(t)->name_off;
> +               if (!from_ref && !name_off && btf_vlen(t))
> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
> +                                  btf_enum64(t)->name_off :
> +                                  btf_enum(t)->name_off;
>
>                 return btf__name_by_offset(btf, name_off);
>         }
>
> Thanks,
> Daniel
>
Andrii Nakryiko Sept. 3, 2024, 4:51 p.m. UTC | #3 
On Mon, Sep 2, 2024 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
> > From: Mykyta Yatsenko <yatsenko@meta.com>
> >
> > Wrong function is used to access the first enum64 element.
> > Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> >
> > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> > ---
> >   tools/bpf/bpftool/btf.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index 6789c7a4d5ca..b0f12c511bb3 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
> >       const struct btf_type *t = btf__type_by_id(btf, index);
> >
> >       switch (btf_kind(t)) {
> > -     case BTF_KIND_ENUM:
> > -     case BTF_KIND_ENUM64: {
> > +     case BTF_KIND_ENUM: {
> >               int name_off = t->name_off;
> >
> >               /* Use name of the first element for anonymous enums if allowed */
> > -             if (!from_ref && !t->name_off && btf_vlen(t))
> > +             if (!from_ref && !name_off && btf_vlen(t))
> >                       name_off = btf_enum(t)->name_off;
> >
> >               return btf__name_by_offset(btf, name_off);
> >       }
>
> Small nit, could we consolidate the logic into the below? Still somewhat nicer
> than duplicating all of the rest.
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..aae6f5262c6a 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>                  int name_off = t->name_off;
>
>                  /* Use name of the first element for anonymous enums if allowed */
> -               if (!from_ref && !t->name_off && btf_vlen(t))
> -                       name_off = btf_enum(t)->name_off;
> +               if (!from_ref && !name_off && btf_vlen(t))
> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?

Just fyi for the future (I don't think we need to fix this or anything
like that), but using BTF_KIND_xxx constants in btf_kind(t)
comparisons should be rare. Libbpf provides a full set of shorter
btf_is_xxx(t) helpers for this. So this would be just
`btf_is_enum64(t)`. What you did is not wrong, it's just more
open-coded and verbose.

> +                                  btf_enum64(t)->name_off :
> +                                  btf_enum(t)->name_off;
>
>                  return btf__name_by_offset(btf, name_off);
>          }
>
> Thanks,
> Daniel
Daniel Borkmann Sept. 3, 2024, 6:27 p.m. UTC | #4 
On 9/3/24 6:51 PM, Andrii Nakryiko wrote:
> On Mon, Sep 2, 2024 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
>>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>>
>>> Wrong function is used to access the first enum64 element.
>>> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
>>>
>>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>>> ---
>>>    tools/bpf/bpftool/btf.c | 13 ++++++++++---
>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>> index 6789c7a4d5ca..b0f12c511bb3 100644
>>> --- a/tools/bpf/bpftool/btf.c
>>> +++ b/tools/bpf/bpftool/btf.c
>>> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>>>        const struct btf_type *t = btf__type_by_id(btf, index);
>>>
>>>        switch (btf_kind(t)) {
>>> -     case BTF_KIND_ENUM:
>>> -     case BTF_KIND_ENUM64: {
>>> +     case BTF_KIND_ENUM: {
>>>                int name_off = t->name_off;
>>>
>>>                /* Use name of the first element for anonymous enums if allowed */
>>> -             if (!from_ref && !t->name_off && btf_vlen(t))
>>> +             if (!from_ref && !name_off && btf_vlen(t))
>>>                        name_off = btf_enum(t)->name_off;
>>>
>>>                return btf__name_by_offset(btf, name_off);
>>>        }
>>
>> Small nit, could we consolidate the logic into the below? Still somewhat nicer
>> than duplicating all of the rest.
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 6789c7a4d5ca..aae6f5262c6a 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>>                   int name_off = t->name_off;
>>
>>                   /* Use name of the first element for anonymous enums if allowed */
>> -               if (!from_ref && !t->name_off && btf_vlen(t))
>> -                       name_off = btf_enum(t)->name_off;
>> +               if (!from_ref && !name_off && btf_vlen(t))
>> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
> 
> Just fyi for the future (I don't think we need to fix this or anything
> like that), but using BTF_KIND_xxx constants in btf_kind(t)
> comparisons should be rare. Libbpf provides a full set of shorter
> btf_is_xxx(t) helpers for this. So this would be just
> `btf_is_enum64(t)`. What you did is not wrong, it's just more
> open-coded and verbose.

Noted, that would have been better agree.

Thanks,
Daniel
diff mbox series

Patch

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..b0f12c511bb3 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -557,16 +557,23 @@  static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
 	const struct btf_type *t = btf__type_by_id(btf, index);
 
 	switch (btf_kind(t)) {
-	case BTF_KIND_ENUM:
-	case BTF_KIND_ENUM64: {
+	case BTF_KIND_ENUM: {
 		int name_off = t->name_off;
 
 		/* Use name of the first element for anonymous enums if allowed */
-		if (!from_ref && !t->name_off && btf_vlen(t))
+		if (!from_ref && !name_off && btf_vlen(t))
 			name_off = btf_enum(t)->name_off;
 
 		return btf__name_by_offset(btf, name_off);
 	}
+	case BTF_KIND_ENUM64: {
+		int name_off = t->name_off;
+
+		if (!from_ref && !name_off && btf_vlen(t))
+			name_off = btf_enum64(t)->name_off;
+
+		return btf__name_by_offset(btf, name_off);
+	}
 	case BTF_KIND_ARRAY:
 		return btf_type_sort_name(btf, btf_array(t)->type, true);
 	case BTF_KIND_TYPE_TAG: