Em Tue, Feb 26, 2019 at 09:14:57AM -0800, Andrii Nakryiko escreveu:
> On Tue, Feb 26, 2019 at 5:12 AM Arnaldo Carvalho de Melo
> <arnaldo.melo@xxxxxxxxx> wrote:
> >
> > Em Mon, Feb 25, 2019 at 01:36:57PM -0800, Andrii Nakryiko escreveu:
> > > 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>
> > > >
> > > > > [386075] INT long unsigned int size=8 bit_offset=0 nr_bits=64 encoding=(none)
> > > > > Cannot open libc-2.28.so.debug
> > > > > Failed to encode BTF
> > > >
> > > > I've tried the same on libc-2.26.so.debug and it worked.
> > > >
> > > > It failes after BTF encoding and deduping is done, exactly when it
> > > > tries to re-open libc-2.28.so.debug to write out new contents. Not
> > > > sure why it would fail, as it seems to be just plain open() call, but
> > > > the fact that we both can repro this means there is something there.
> > > >
> > > > It also seems that libc-2.28.so.debug is corrupted:
> > > >
> > > > $ readelf -n /home/andriin/local/btf/libc-2.26.so.debug | grep
> > > > 'Unknown note type' | wc -l
> > > > 0
> > > > $ readelf -n /home/andriin/local/btf/libc-2.28.so.debug | grep
> > > > 'Unknown note type' | wc -l
> > > > 17953
> > > >
> > >
> > > Ignore everything I've said (though 'Unknown note type' issue is
> > > strange), for me it was just permissions issues.
> >
> > yeah, we need to fix up these messages, I did the chmod here and got
> > further.
> >
> > [acme@quaco pahole]$ ls -la libc-2.28.so.debug
> > -r--r--r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug
> > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug
> > Cannot open libc-2.28.so.debug
> > Failed to encode BTF
> > [acme@quaco pahole]$ chmod +w libc-2.28.so.debug
> > [acme@quaco pahole]$ ls -la libc-2.28.so.debug
> > -rw-rw-r--. 1 acme acme 17189368 Feb 25 17:04 libc-2.28.so.debug
> > [acme@quaco pahole]$ pahole -J libc-2.28.so.debug
> > [acme@quaco pahole]$
> >
> > And I get the same warnings.
> >
> > base_type__name_to_size: base_type _Float128
> >
> > If we just add _Float128 to the base_type__name_to_size arrays I think
> > its good enough for pretty printing, i.e. we won't know from encoding
> > that this is a float or double, as CTF and DWARF can, but for printing
> > purposes, even regenerating the type for consumption by other tools, it
> > should just work, no?
>
> With "btf_loader: Simplify fixup code by relying on BTF data more" patch,
> btf_loader doesn't rely on this mapping at all. So if there is any other
> base type we didn't foresee, BTF output should still be correct.
>
> > So, two bugs, the bit_size for "long double" is 128, not 64, and if we
> > add an entry for _Float128, then btfdiff is clean for libc, without any
> > further work on properly supporting float types as in CTF or DWARF:
>
> My worry with this table (and one of the reasons for that patch to not use
> it at all) was that those sizeof() sizes might be correct for the
> architecture on which pahole was built, but not necessarily on the target
> arch for which ELF file was built. E.g., I don't know if it's true that
Right, that is why there are some entries with size zero, meaning they
need to be calculated based on the size of a pointer in the arch where
it was produced, not the one where it is beind processed.
But yeah, I even made a note about that in the cset I added fixing the
bit_size for "long double".
As you said, I have to look and see if we can use the same logic in DWARF
that you used for BTF and in the end just ditch this table completely.
- Arnaldo
> long double is always 128-bit on all architectures, so relying on that
> table might still break output for CTF and DWARF (but not BTF, because it
> takes bitness from BTF data itself).
>
> But otherwise it's still good thing to complete that table.
>
> >
> > [acme@quaco pahole]$ btfdiff libc-2.28.so.debug
> > [acme@quaco pahole]$ git diff
> > diff --git a/dwarves.c b/dwarves.c
> > index adc411a8279d..48a018a9c354 100644
> > --- a/dwarves.c
> > +++ b/dwarves.c
> > @@ -168,11 +168,12 @@ static struct base_type_name_to_size {
> > { .name = "double double", .size = 64, },
> > { .name = "single float", .size = 32, },
> > { .name = "float", .size = 32, },
> > - { .name = "long double", .size = 64, },
> > - { .name = "long double long double", .size = 64, },
> > + { .name = "long double", .size = sizeof(long double) * 8, },
> > + { .name = "long double long double", .size = sizeof(long double) * 8, },
> > { .name = "__int128", .size = 128, },
> > { .name = "unsigned __int128", .size = 128, },
> > { .name = "__int128 unsigned", .size = 128, },
> > + { .name = "_Float128", .size = 128, },
> > { .name = NULL },
> > };
> >
> > [acme@quaco pahole]$
> >
> > I'll add a series of patches fixing up the types that don't have encoded
> > in its name the size in bits, so that it works on all arches, one more
> > for _Float128, should be enough for now, right?
> >
> > - Arnaldo
> >
> > > $ chmod +rw /home/andriin/local/btf/libc-2.28.so.debug
> > >
> > > $ ~/local/pahole/build/pahole -J ~/local/btf/libc-2.28.so.debug
> > >
> > > $ PAHOLE=~/local/pahole/build/pahole ./btfdiff ~/local/btf/libc-2.28.so.debug
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > base_type__name_to_size: base_type _Float128
> > > class__fixup_btf_bitfields: unknown base type name "_Float128"!
> > > --- /tmp/btfdiff.dwarf.aV4wSL 2019-02-25 13:31:54.787923673 -0800
> > > +++ /tmp/btfdiff.btf.22NQmJ 2019-02-25 13:31:54.802923668 -0800
> > > @@ -461,9 +461,15 @@ struct La_x86_64_retval {
> > > uint64_t lrv_rdx; /* 8 8 */
> > > La_x86_64_xmm lrv_xmm0; /* 16 16 */
> > > La_x86_64_xmm lrv_xmm1; /* 32 16 */
> > > - long double lrv_st0; /* 48 16 */
> > > + long double lrv_st0; /* 48 8 */
> > > +
> > > + /* XXX 8 bytes hole, try to pack */
> > > +
> > > /* --- cacheline 1 boundary (64 bytes) --- */
> > > - long double lrv_st1; /* 64 16 */
> > > + long double lrv_st1; /* 64 8 */
> > > +
> > > + /* XXX 8 bytes hole, try to pack */
> > > +
> > > La_x86_64_vector lrv_vector0; /* 80 64 */
> > > /* --- cacheline 2 boundary (128 bytes) was 16 bytes ago --- */
> > > La_x86_64_vector lrv_vector1; /* 144 64 */
> > > @@ -472,6 +478,7 @@ struct La_x86_64_retval {
> > > __int128 lrv_bnd1; /* 224 16 */
> > >
> > > /* size: 240, cachelines: 4, members: 10 */
> > > + /* sum members: 224, holes: 2, sum holes: 16 */
> > > /* last cacheline: 48 bytes */
> > > };
> > > struct r_debug {
> > > @@ -2044,7 +2051,7 @@ union ieee754_float {
> > > } ieee_nan; /* 0 4 */
> > > };
> > > union ieee854_long_double {
> > > - long double d; /* 0 16 */
> > > + long double d; /* 0 8 */
> > > struct {
> > > unsigned int mantissa1:32; /* 0: 0 4 */
> > > unsigned int mantissa0:32; /* 4: 0 4 */
> > > @@ -2141,7 +2148,7 @@ struct ucontext_t {
> > > /* last cacheline: 8 bytes */
> > > };
> > > union ieee854_float128 {
> > > - _Float128 d; /* 0 16 */
> > > + _Float128 d; /* 0 0 */
> > > struct {
> > > unsigned int mantissa3:32; /* 0: 0 4 */
> > > unsigned int mantissa2:32; /* 4: 0 4 */
> > > @@ -2219,7 +2226,7 @@ union printf_arg {
> > > long unsigned int pa_u_long_int; /* 0 8 */
> > > long long unsigned int pa_u_long_long_int; /* 0 8 */
> > > double pa_double; /* 0 8 */
> > > - long double pa_long_double; /* 0 16 */
> > > + long double pa_long_double; /* 0 8 */
> > > const char * pa_string; /* 0 8 */
> > > const wchar_t * pa_wstring; /* 0 8 */
> > > void * pa_pointer; /* 0 8 */
> > >
> > > Seems like "long double" size is invalid for BTF. And we should
> > > probably add "_Float128" as another "well known" base type name?
> > >
> > > Though for the latter, it probably would be better to resolve base
> > > type size using BTF data itself.
> > >
> > > > > [acme@quaco pahole]$
> >
> > --
> >
> > - Arnaldo
--
- Arnaldo
