Re: [PATCH pahole 0/3] fix handling of bitfields in btf_loader

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Fri, Feb 22, 2019 at 03:23:52PM -0800, Andrii Nakryiko escreveu:
> On Fri, Feb 22, 2019 at 2:02 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> > Em Wed, Feb 20, 2019 at 12:57:29PM -0800, Andrii Nakryiko escreveu:
> > > This patchset fixes the logic of determining bitfield_offsets and
> > > initial bit_offset when using BTF type information. It eliminates all
> > > the remaining discrepancies, when doing btfdiff on vmlinux image, module
> > > two instances of incorrectly reporting struct member type name when
> > > bitfield is the very first field in a struct, which is only happening
> > > when using DWARF data.
> > >
> > > Patch #1 makes btfdiff script easier to use during local development.
> > >
> > > Patch #2 fixes list of known base type names to handle clang-generated
> > > type descriptions better.
> > >
> > > Patch #3 fixes bitfield handling logic in btf_loader.
> >
> > Thanks a lot! Applied.
> >
> > And here I see no differences in my vmlinux:
> >
> 
> <SNIP>
> 
> >
> > Which ones where these: "module two instances of incorrectly reporting struct
> > member type name when bitfield is the very first field in a struct, which is
> > only happening when using DWARF data." ?
> 
> $ ./btfdiff /tmp/vmlinux4
> --- /tmp/btfdiff.dwarf.GIVfpr   2019-02-20 12:18:29.138788970 -0800
> +++ /tmp/btfdiff.btf.c3x2KY     2019-02-20 12:18:29.351786365 -0800
> @@ -16884,7 +16884,7 @@ struct pebs_record_nhm {
>  };
>  union hsw_tsx_tuning {
>         struct {
> -               unsigned int       cycles_last_block:32; /*     0: 0  4 */
> +               u32                cycles_last_block:32; /*     0: 0  4 */

$ vim hsw_tsx_tuning.c
$ gcc -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-gcc
$ pahole -J hsw_tsx_tuning
pahole: hsw_tsx_tuning: No such file or directory
$ pahole -J hsw_tsx_tuning-gcc
$
$ btfdiff hsw_tsx_tuning-gcc
--- /tmp/btfdiff.dwarf.ErXffk	2019-02-25 10:26:56.863625697 -0300
+++ /tmp/btfdiff.btf.Y5EDdM	2019-02-25 10:26:56.864625706 -0300
@@ -1,6 +1,6 @@
 union hsw_tsx_tuning {
 	struct {
-		unsigned int       cycles_last_block:32; /*     0: 0  4 */
+		u32                cycles_last_block:32; /*     0: 0  4 */
 		u32                hle_abort:1;        /*     4:31  4 */
 		u32                rtm_abort:1;        /*     4:30  4 */
 		u32                instruction_abort:1; /*     4:29  4 */
$

Reproduced. 

 <2><5c>: Abbrev Number: 5 (DW_TAG_member)
    <5d>   DW_AT_name        : (indirect string, offset: 0x23): cycles_last_block
    <61>   DW_AT_decl_file   : 1
    <62>   DW_AT_decl_line   : 8
    <63>   DW_AT_decl_column : 13
    <64>   DW_AT_type        : <0x40>
    <68>   DW_AT_byte_size   : 4
    <69>   DW_AT_bit_size    : 32
    <6a>   DW_AT_bit_offset  : 0
    <6b>   DW_AT_data_member_location: 0
 <1><40>: Abbrev Number: 2 (DW_TAG_typedef)
    <41>   DW_AT_name        : u32
    <45>   DW_AT_decl_file   : 1
    <46>   DW_AT_decl_line   : 4
    <47>   DW_AT_decl_column : 22
    <48>   DW_AT_type        : <0x4c>
 <1><4c>: Abbrev Number: 3 (DW_TAG_base_type)
    <4d>   DW_AT_byte_size   : 4
    <4e>   DW_AT_encoding    : 7        (unsigned)
    <4f>   DW_AT_name        : (indirect string, offset: 0x16): unsigned int

So yeah, the BTF encoder/decoder is working just fine, the problem is in
pahole's DWARF code, lemme see...

>                 u32                hle_abort:1;        /*     4:31  4 */
>                 u32                rtm_abort:1;        /*     4:30  4 */
>                 u32                instruction_abort:1; /*     4:29  4 */
> @@ -26154,7 +26154,7 @@ struct acpi_device_power {
>         /* last cacheline: 40 bytes */
>  };
>  struct acpi_device_perf_flags {
> -       unsigned char              reserved:8;           /*     0: 0  1 */
> +       u8                         reserved:8;           /*     0: 0  1 */
> 
>         /* size: 1, cachelines: 1, members: 1 */
>         /* last cacheline: 1 bytes */
> 
> For hsw_tsx_tuning, it is defined in sources like this:
> 
> $ cat hsw_tsx_tuning.c
> typedef unsigned long long u64;
> typedef unsigned int u32;
> 
> union hsw_tsx_tuning {
>     struct {
>         u32 cycles_last_block     : 32,
>             hle_abort             : 1,
>             rtm_abort             : 1,
>             instruction_abort     : 1,
>             non_instruction_abort : 1,
>             retry                 : 1,
>             data_conflict         : 1,
>             capacity_writes       : 1,
>             capacity_reads        : 1;
>     };
>     u64        value;
> };
> 
> int main() {
>     union hsw_tsx_tuning t;
>     return 0;
> }
> 
> Building same defition with gcc and clang reveals some more info.
> 
> $ cc -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-gcc &&
> ~/local/pahole/build/pahole -J hsw_tsx_tuning-gcc
> $ clang -g -c hsw_tsx_tuning.c -o hsw_tsx_tuning-clang &&
> ~/local/pahole/build/pahole -J hsw_tsx_tuning-clang
> 
> GCC-generated DWARF/BTF exposes this bug:
> 
> $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-gcc
> --- /tmp/btfdiff.dwarf.khRiFg   2019-02-22 15:18:46.768858141 -0800
> +++ /tmp/btfdiff.btf.jqdEsR     2019-02-22 15:18:46.770858140 -0800
> @@ -1,6 +1,6 @@
>  union hsw_tsx_tuning {
>         struct {
> -               unsigned int       cycles_last_block:32; /*     0: 0  4 */
> +               u32                cycles_last_block:32; /*     0: 0  4 */
>                 u32                hle_abort:1;        /*     4:31  4 */
>                 u32                rtm_abort:1;        /*     4:30  4 */
>                 u32                instruction_abort:1; /*     4:29  4 */
> 
> But the one generated by clang doesn't:
> 
> $ PAHOLE=~/local/pahole/build/pahole ~/local/pahole/btfdiff hsw_tsx_tuning-clang
> 
> The only difference is that GCC correctly marks cycles_last_block as
> bitfield, while clang optimizes it to stand-alone u32.
> 
> $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-gcc
> union hsw_tsx_tuning {
>         struct {
>                 u32                cycles_last_block:32; /*     0: 0  4 */
>                 u32                hle_abort:1;        /*     4:31  4 */
>                 u32                rtm_abort:1;        /*     4:30  4 */
>                 u32                instruction_abort:1; /*     4:29  4 */
>                 u32                non_instruction_abort:1; /*     4:28  4 */
>                 u32                retry:1;            /*     4:27  4 */
>                 u32                data_conflict:1;    /*     4:26  4 */
>                 u32                capacity_writes:1;  /*     4:25  4 */
>                 u32                capacity_reads:1;   /*     4:24  4 */
>         };                                             /*     0     8 */
>         u64                        value;              /*     0     8 */
> };
> 
> $ ~/local/pahole/build/pahole -F btf hsw_tsx_tuning-clang
> union hsw_tsx_tuning {
>         struct {
>                 u32                cycles_last_block;  /*     0     4 */
>                 u32                hle_abort:1;        /*     4:31  4 */
>                 u32                rtm_abort:1;        /*     4:30  4 */
>                 u32                instruction_abort:1; /*     4:29  4 */
>                 u32                non_instruction_abort:1; /*     4:28  4 */
>                 u32                retry:1;            /*     4:27  4 */
>                 u32                data_conflict:1;    /*     4:26  4 */
>                 u32                capacity_writes:1;  /*     4:25  4 */
>                 u32                capacity_reads:1;   /*     4:24  4 */
>         };                                             /*     0     8 */
>         u64                        value;              /*     0     8 */
> };
> 
> I've spent a bit of time debugging this, but didn't get far, as I'm
> pretty unfamiliar with overall flow of decoders, I was hoping this can
> give you some idea where to look, though.
> 
> >
> > - Arnaldo

-- 

- Arnaldo