Re: [PATCH bpf-next v2 10/18] libbpf: Add enum64 relocation support

[Yonghong Song <yhs@fb.com>]

On 5/17/22 4:32 PM, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> The enum64 relocation support is added. The bpf local type
>> could be either enum or enum64 and the remote type could be
>> either enum or enum64 too. The all combinations of local enum/enum64
>> and remote enum/enum64 are supported.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/btf.h       |  7 ++++++
>>   tools/lib/bpf/libbpf.c    |  7 +++---
>>   tools/lib/bpf/relo_core.c | 49 ++++++++++++++++++++++++++-------------
>>   3 files changed, 44 insertions(+), 19 deletions(-)
>>
> 
> [...]
> 
>>          memset(targ_spec, 0, sizeof(*targ_spec));
>>          targ_spec->btf = targ_btf;
>> @@ -494,18 +498,22 @@ static int bpf_core_spec_match(struct bpf_core_spec *local_spec,
>>
>>          if (core_relo_is_enumval_based(local_spec->relo_kind)) {
>>                  size_t local_essent_len, targ_essent_len;
>> -               const struct btf_enum *e;
>>                  const char *targ_name;
>>
>>                  /* has to resolve to an enum */
>>                  targ_type = skip_mods_and_typedefs(targ_spec->btf, targ_id, &targ_id);
>> -               if (!btf_is_enum(targ_type))
>> +               if (!btf_type_is_any_enum(targ_type))
> 
> just noticed this discrepancy, can you please rename
> s/btf_type_is_any_enum/btf_is_any_enum/ so it's consistent with
> btf_is_enum and btf_is_enum64?

okay.

> 
>>                          return 0;
>>
>>                  local_essent_len = bpf_core_essential_name_len(local_acc->name);
>>
>> -               for (i = 0, e = btf_enum(targ_type); i < btf_vlen(targ_type); i++, e++) {
>> -                       targ_name = btf__name_by_offset(targ_spec->btf, e->name_off);
>> +               for (i = 0; i < btf_vlen(targ_type); i++) {
>> +                       if (btf_is_enum(targ_type))
>> +                               name_off = btf_enum(targ_type)[i].name_off;
>> +                       else
>> +                               name_off = btf_enum64(targ_type)[i].name_off;
>> +
>> +                       targ_name = btf__name_by_offset(targ_spec->btf, name_off);
>>                          targ_essent_len = bpf_core_essential_name_len(targ_name);
>>                          if (targ_essent_len != local_essent_len)
>>                                  continue;
>> @@ -681,7 +689,7 @@ static int bpf_core_calc_field_relo(const char *prog_name,
>>                  break;
>>          case BPF_CORE_FIELD_SIGNED:
>>                  /* enums will be assumed unsigned */
> 
> we don't have to assume anymore, right? let's use kflag for btf_is_any_enum()?

old comment is not accurate any more, will remove.

> 
>> -               *val = btf_is_enum(mt) ||
>> +               *val = btf_type_is_any_enum(mt) ||
>>                         (btf_int_encoding(mt) & BTF_INT_SIGNED);
>>                  if (validate)
>>                          *validate = true; /* signedness is never ambiguous */
> 
> [...]
> 
>> @@ -1089,10 +1097,19 @@ int bpf_core_format_spec(char *buf, size_t buf_sz, const struct bpf_core_spec *s
>>
>>          if (core_relo_is_enumval_based(spec->relo_kind)) {
>>                  t = skip_mods_and_typedefs(spec->btf, type_id, NULL);
>> -               e = btf_enum(t) + spec->raw_spec[0];
>> -               s = btf__name_by_offset(spec->btf, e->name_off);
>> +               if (btf_is_enum(t)) {
>> +                       const struct btf_enum *e;
>>
>> -               append_buf("::%s = %u", s, e->val);
>> +                       e = btf_enum(t) + spec->raw_spec[0];
>> +                       s = btf__name_by_offset(spec->btf, e->name_off);
>> +                       append_buf("::%s = %u", s, e->val);
>> +               } else {
>> +                       const struct btf_enum64 *e;
>> +
>> +                       e = btf_enum64(t) + spec->raw_spec[0];
>> +                       s = btf__name_by_offset(spec->btf, e->name_off);
>> +                       append_buf("::%s = %llu", s, btf_enum64_value(e));
> 
> nit: we do have a sign bit now, so maybe let's print %lld or %llu
> (same for %d and %u above)? btw, please cast (unsigned long long) here

will do.

> 
>> +               }
>>                  return len;
>>          }
>>
>> --
>> 2.30.2
>>