On 2/26/19 9:22 AM, Andrii Nakryiko wrote:
> On Mon, Feb 25, 2019 at 10:03 PM Yonghong Song <yhs@xxxxxx> wrote:
>>
>>
>>
>> On 2/25/19 9:45 PM, Andrii Nakryiko wrote:
>>> On Mon, Feb 25, 2019 at 9:38 PM Yonghong Song <yhs@xxxxxx> wrote:
>>>>
>>>>
>>>>
>>>> On 2/25/19 1:36 PM, Andrii Nakryiko wrote:
>>>>> On Mon, Feb 25, 2019 at 12:51 PM Andrii Nakryiko
>>>>> <andrii.nakryiko@xxxxxxxxx> wrote:
>>>>>>
>>>>>> On Mon, Feb 25, 2019 at 12:08 PM Arnaldo Carvalho de Melo
>>>>>> <arnaldo.melo@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> Em Mon, Feb 25, 2019 at 11:42:29AM -0800, Andrii Nakryiko escreveu:
>>>>>>>> On Mon, Feb 25, 2019 at 11:00 AM Arnaldo Carvalho de Melo> <arnaldo.melo@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>>> Em Mon, Feb 25, 2019 at 01:02:39PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>>>>>>>> So yeah, the BTF encoder/decoder is working just fine, the problem is in
>>>>>>>>>> pahole's DWARF code, lemme see...
>>>>>>>>>
>>>>>>>>> Please try the patch below, for me btfdiff continues to show no diff for
>>>>>>>>> all types in my vmlinux and now it also produces the same output for
>>>>>>>>> when the first element of a bitfield has its bit_size equal to its
>>>>>>>>> byte_size * 8:
>>>>>>>>
>>>>>>>> Yes, this fixes all the issues I've seen. btfdiff output is now empty
>>>>>>>> for my kernel image. Thanks for quick fix!
>>>>>>>>
>>>>>>>> Reviewed-by: Andrii Nakryiko <andriin@xxxxxx>
>>>>>>>
>>>>>>> Thanks, just for some extra testing I tried encoding BTF for glibc 2.28:
>>>>>>>
>>>>>>> [acme@quaco pahole]$ pahole -JV libc-2.28.so.debug
>>>>>>> <BIG snip>
>>>>>>
>>>>>> <snip>
>>>>>>
>
> <snip>
>
>>>>>
>>>>> Seems like "long double" size is invalid for BTF. And we should
>>>>> probably add "_Float128" as another "well known" base type name?
>>>>
>>>> BPF does not support floating point instructions. As the result,
>>>> BTF design does not contain floating point types.
>>>>
>>>> Looks like kernel also uses float types (in certain drivers and arch's),
>>>> but types referred by bpf program mostly will not contain float types.
>>>>
>>>> Currently, both llvm and pahole will ignore floating types, so
>>>> from type comparison point of view, we will be fine.
>>>> The issue may come up with people using vmlinux BTF inside the
>>>> kernel for different purpose where a float type needs to be
>>>> specified, e.g., for a floating point function argument in ftrace.
>>>> But this is rare too.
>>>>
>>>> I am not 100% sure whether pahole dwarf/btf parity is considered
>>>> as a strong reason to add floating point type support in BTF or not.
>>>
>>> Isn't float and double just another base type with corresponding name?
>>> Fundamentally, how is it different from int or long? It's still just a
>>> collection of bits in 32-bit, 64-bit, etc number, except interpreted
>>> differently. Do you mean that for BTF to properly support floats,
>>> we'll need to extends set of possible encodings in INT type (in
>>> addition to today's CHAR, SIGNED and UNSIGNED)?
>>
>> Probably another kind like BTF_KIND_FLOAT with additional u32 (in
>> addition to btf_type) indicating the size (4 bytes, 8 bytes and 16 bytes).
>
> This sounds exactly like current format of BTF_KIND_INT. What
> BTF_KIND_FLOAT will have that can't be represented by BTF_KIND_INT?
> Wouldn't it be equivalent to just adding BTF_INT_FLOAT in addition to
> existing BTF_INT_SIGNED, BTF_INT_CHAR, BTF_INT_BOOL to
> BTF_INT_ENCODING? Btw, while BTF_INT_SIGNED seems like it deserves
> separate bit, because it's mostly orthogonal to CHAR vs INT vs FLOAT
> vs (not so much) BOOL, BTF_INT_CHAR and BTF_INT_BOOL take two
> independent bits seems excessive. You can't have BOOL and CHAR, right?
> So why don't we just repurpose bits 1 and 2 to be a small 4-value
> enum:
>
> enum btf_int_encoding {
> INT = 0,
> CHAR = 1,
> BOOL = 2,
> FLOAT = 3,
> };
>
> This is backwards compatible and covers all floats (long double vs
> double vs float is just a question of bitness).
>
> Am I missing something here?
float is not an "int". Encoding "float" in INT_KIND does not sound right
in my opinion.
>
>>
>>>
>>>>
>>>>>
>>>>> Though for the latter, it probably would be better to resolve base
>>>>> type size using BTF data itself.
>>>>>
>>>>>>> [acme@quaco pahole]$
