Re: [PATCH bpf-next v2 06/18] libbpf: Add enum64 deduplication support

[Yonghong Song <yhs@fb.com>]

On 5/16/22 5:28 PM, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add enum64 deduplication support. BTF_KIND_ENUM64 handling
>> is very similar to BTF_KIND_ENUM.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/btf.c | 55 +++++++++++++++++++++++++++++++++------------
>>   tools/lib/bpf/btf.h |  5 +++++
>>   2 files changed, 46 insertions(+), 14 deletions(-)
>>
> 
> [...]
> 
>> +static bool btf_equal_enum64_val(struct btf_type *t1, struct btf_type *t2)
>> +{
>> +       const struct btf_enum64 *m1, *m2;
>> +       __u16 vlen = btf_vlen(t1);
>> +       int i;
>> +
>> +       m1 = btf_enum64(t1);
>> +       m2 = btf_enum64(t2);
>> +       for (i = 0; i < vlen; i++) {
>> +               if (m1->name_off != m2->name_off || m1->val_lo32 != m2->val_lo32 ||
>> +                   m1->val_hi32 != m2->val_hi32)
>> +                       return false;
>> +               m1++;
>> +               m2++;
>> +       }
>> +       return true;
>> +}
>> +
>> +/* Check structural equality of two ENUMs. */
>> +static bool btf_equal_enum_or_enum64(struct btf_type *t1, struct btf_type *t2)
> 
> I find this helper quite confusing. It implies it can compare any enum
> or enum64 with each other, but it really allows only enum vs enum and
> enum64 vs enum64 (as it should!). Let's keep
> btf_equal_enum()/btf_compat_enum() completely intact and add
> btf_equal_enum64()/btf_compat_enum64() separately (few lines of
> copy-pasted code is totally fine to keep them separate, IMO). See
> below.

I debate with myself about whether I should use separate functions or
use one function for both enum/enum64. My current approach will have
less code changes. But I can do what you suggested to have separate
functions for enum and enum64. This will apply to btf_compat_enum
as well.

> 
>> +{
>> +       if (!btf_equal_common(t1, t2))
>> +               return false;
>> +
>> +       if (btf_is_enum(t1))
>> +               return btf_equal_enum32_val(t1, t2);
>> +       return btf_equal_enum64_val(t1, t2);
>> +}
>> +
>>   static inline bool btf_is_enum_fwd(struct btf_type *t)
>>   {
>> -       return btf_is_enum(t) && btf_vlen(t) == 0;
>> +       return btf_type_is_any_enum(t) && btf_vlen(t) == 0;
>>   }
>>
[...]