Re: [PATCH bpf-next 2/6] libbpf: Add BTF_KIND_FLOAT support

[Yonghong Song <yhs@fb.com>]

On 2/18/21 5:41 AM, Ilya Leoshkevich wrote:
> On Wed, 2021-02-17 at 22:58 -0800, Yonghong Song wrote:
>> On 2/15/21 5:12 PM, Ilya Leoshkevich wrote:
>>> The logic follows that of BTF_KIND_INT most of the time.
>>> Sanitization
>>> replaces BTF_KIND_FLOATs with equally-sized BTF_KIND_INTs on older
>>> kernels.
>>>
>>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>>> ---
>>>    tools/lib/bpf/btf.c             | 44
>>> +++++++++++++++++++++++++++++++++
>>>    tools/lib/bpf/btf.h             |  8 ++++++
>>>    tools/lib/bpf/btf_dump.c        |  4 +++
>>>    tools/lib/bpf/libbpf.c          | 29 +++++++++++++++++++++-
>>>    tools/lib/bpf/libbpf.map        |  5 ++++
>>>    tools/lib/bpf/libbpf_internal.h |  2 ++
>>>    6 files changed, 91 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>>> index d9c10830d749..07a30e98c3de 100644
>>> --- a/tools/lib/bpf/btf.c
>>> +++ b/tools/lib/bpf/btf.c
> 
> [...]
> 
>>> @@ -2373,6 +2377,42 @@ int btf__add_datasec(struct btf *btf, const
>>> char *name, __u32 byte_sz)
>>>          return btf_commit_type(btf, sz);
>>>    }
>>>    
>>> +/*
>>> + * Append new BTF_KIND_FLOAT type with:
>>> + *   - *name* - non-empty, non-NULL type name;
>>> + *   - *sz* - size of the type, in bytes;
>>> + * Returns:
>>> + *   - >0, type ID of newly added BTF type;
>>> + *   - <0, on error.
>>> + */
>>> +int btf__add_float(struct btf *btf, const char *name, size_t
>>> byte_sz)
>>> +{
>>> +       struct btf_type *t;
>>> +       int sz, name_off;
>>> +
>>> +       /* non-empty name */
>>> +       if (!name || !name[0])
>>> +               return -EINVAL;
>>
>> Do we want to ensure byte_sz to be 2/4/8/16?
>> Currently, the int type supports 1/2/4/8/16.
>>
>> In LLVM, the following are supported float types:
>>
>>     case BuiltinType::Half:
>>     case BuiltinType::Float:
>>     case BuiltinType::LongDouble:
>>     case BuiltinType::Float16:
>>     case BuiltinType::BFloat16:
>>     case BuiltinType::Float128:
>>     case BuiltinType::Double:
> 
> There can be 80-bit floats on x86:
> 
> #include <stdio.h>
> int main() { printf("%zu\n", sizeof(long double)); }
> 
> prints 12 when compiled with -m32 (not 10 due to alignment I assume).

Wow, I do not know we have long double float size = 12 for m32.
In this case, maybe we can enforce float size of 2 and multiple of 4?
I still like some enforcement, arbitrary float size seems wrong to me.

> 
> I guess this now completely kills the idea with sanitizing FLOATs to
> equally-sized INTs...

My previous email suggests to use "const int" for sanitization.
In kernel, we did not enforce the "int" type size as long as it
is equal to or greater than the one represented by bits.

         if (BITS_ROUNDUP_BYTES(nr_bits) > t->size) {
                 btf_verifier_log_type(env, t, "nr_bits exceeds type_size");
                 return -EINVAL;
         }

But in libbpf, we do want to the int size to be power-of-2.

         /* byte_sz must be power of 2 */
         if (!byte_sz || (byte_sz & (byte_sz - 1)) || byte_sz > 16)
                 return -EINVAL;

When people dump in-kernel sanitized btf and they may see
an int with size 12, which will be weird.

So maybe "const char_array" is the best choice here.

> 
> [...]
>