dwarves.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
       [not found]     ` <DEC4CC81-88CE-4476-A631-2BBB6E922F5C@gmail.com>
@ 2020-09-30  6:52       ` Hao Luo
  2020-10-01 15:47         ` Alexei Starovoitov
  0 siblings, 1 reply; 8+ messages in thread
From: Hao Luo @ 2020-09-30  6:52 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, Alexei Starovoitov,
	Daniel Borkmann, Martin KaFai Lau, dwarves, Yonghong Song

Arnaldo,

Is this patch ready to be merged into Pahole's master branch? Alexei
is testing the kernel patches that need this patch. Please let me know
if there is anything I can do to help merging.

Thank you,
Hao

On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> >Arnaldo,
> >
> >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> ><acme@kernel.org> wrote:
> >>
> >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> >> > It is found on gcc 8.2 that global percpu variables generate the
> >> > following dwarf entry in the cu where the variable is defined[1].
> >> >
> >> > Take the global variable "bpf_prog_active" defined in
> >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> >> > has two dwarf entries for "bpf_prog_active".
> >> >
> >[...]
> >>
> >> Interesting, here I get, with binutils' readelf:
> >>
> >> [root@quaco perf]# readelf -wi
> >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> >bpf_prog_active
> >> [root@quaco perf]#
> >>
> >> Just one, as:
> >>
> >> [root@quaco perf]# readelf -wi
> >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> >bpf_prog_active
> >>     <f6a5>   DW_AT_decl_file   : 11
> >>     <f6a6>   DW_AT_decl_line   : 1008
> >>     <f6a8>   DW_AT_decl_column : 1
> >>     <f6a9>   DW_AT_type        : <0xcf>
> >>     <f6ad>   DW_AT_external    : 1
> >>     <f6ad>   DW_AT_declaration : 1
> >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> >bpf_stats_enabled_mutex
> >> [root@quaco perf]#
> >>
> >> I get what you have when I use elfutils' readelf:
> >>
> >> [root@quaco perf]# eu-readelf -winfo
> >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> >>              name                 (strp) "bpf_prog_active"
> >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> >> [root@quaco perf]#
> >>
> >> [root@quaco perf]# eu-readelf -winfo
> >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> >\"bpf_prog_active\"
> >>  [  f6a0]    variable             abbrev: 103
> >>              name                 (strp) "bpf_prog_active"
> >>              decl_file            (data1) bpf.h (11)
> >>              decl_line            (data2) 1008
> >>              decl_column          (data1) 1
> >>              type                 (ref4) [    cf]
> >>              external             (flag_present) yes
> >>              declaration          (flag_present) yes
> >>  [  f6ad]    variable             abbrev: 103
> >>              name                 (strp) "bpf_stats_enabled_mutex"
> >> [root@quaco perf]#
> >>
> >> And:
> >>
> >> [root@quaco perf]# eu-readelf -winfo
> >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> >>  [ 1bdf5]    variable             abbrev: 212
> >>              specification        (ref4) [  f6a0]
> >>              decl_file            (data1) syscall.c (1)
> >>              decl_line            (data1) 43
> >>              location             (exprloc)
> >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> >> [root@quaco perf]#
> >>
> >
> >In binutils readelf, there is a extra entry
>
> Not here, tomorrow I'll triple check.
>
> >
> > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> >    <1b24e>   DW_AT_specification: <0xf335>
> >    <1b252>   DW_AT_decl_file   : 1
> >    <1b253>   DW_AT_decl_line   : 43
> >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > (DW_OP_addr: 0)
> >
> >which points to
> >
> > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> >bpf_prog_active
> >
> >It just doesn't have the string 'bpf_prog_active', annotating entry.
> >So eu-readelf and binutils readelf have the same results.
> >
> >> > Note that second DW_TAG_variable entry contains specification that
> >> > points to the first entry.
> >>
> >> So you are not considering the first when encoding since it is just a
> >> DW_AT_declaration, considers the second, as it should be, and then
> >needs
> >> to go see its DW_AT_specification, right?
> >>
> >> Sounds correct, applying, will test further and then push out,
> >>
> >
> >Yes, exactly. The var tags to be considered are those that either have
> >DW_AT_specification or not have DW_AT_declaration. This makes sure
> >btf_encoder works correctly on both old and new gcc.
> >
> >> Thanks,
> >>
> >> - Arnaldo
> >
> >Suggested by Yonghong, I tested this change on a larger set of
> >compilers this time and works correctly. See below.
> >
> >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> >have done that when sending out this patch. The credit goes to
> >Yonghong.
>
> Sure, and I'll add your results with different computers, for the record.
>
> Thanks,
>
> - Arnaldo
> >
> >Thank you,
> >Hao
> >
> >  clang 10:
> >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> >
> >  clang 9:
> >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> >
> >  gcc 10.2
> >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> >
> >  gcc 9.3:
> >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> >
> >  gcc 8
> >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> >
> >  gcc 6.2
> >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> >
> >  gcc 4.9
> >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-09-30  6:52       ` [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification Hao Luo
@ 2020-10-01 15:47         ` Alexei Starovoitov
  2020-10-01 18:24           ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2020-10-01 15:47 UTC (permalink / raw)
  To: Hao Luo, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo
  Cc: Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

Arnaldo,

ping.
Is anything blocking this fix from merging?
The kernel patches are stalled waiting on the pahole.

Thanks
On Tue, Sep 29, 2020 at 11:52 PM Hao Luo <haoluo@google.com> wrote:
>
> Arnaldo,
>
> Is this patch ready to be merged into Pahole's master branch? Alexei
> is testing the kernel patches that need this patch. Please let me know
> if there is anything I can do to help merging.
>
> Thank you,
> Hao
>
> On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
> <arnaldo.melo@gmail.com> wrote:
> >
> >
> >
> > On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> > >Arnaldo,
> > >
> > >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> > ><acme@kernel.org> wrote:
> > >>
> > >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> > >> > It is found on gcc 8.2 that global percpu variables generate the
> > >> > following dwarf entry in the cu where the variable is defined[1].
> > >> >
> > >> > Take the global variable "bpf_prog_active" defined in
> > >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> > >> > has two dwarf entries for "bpf_prog_active".
> > >> >
> > >[...]
> > >>
> > >> Interesting, here I get, with binutils' readelf:
> > >>
> > >> [root@quaco perf]# readelf -wi
> > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > >bpf_prog_active
> > >> [root@quaco perf]#
> > >>
> > >> Just one, as:
> > >>
> > >> [root@quaco perf]# readelf -wi
> > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> > >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > >bpf_prog_active
> > >>     <f6a5>   DW_AT_decl_file   : 11
> > >>     <f6a6>   DW_AT_decl_line   : 1008
> > >>     <f6a8>   DW_AT_decl_column : 1
> > >>     <f6a9>   DW_AT_type        : <0xcf>
> > >>     <f6ad>   DW_AT_external    : 1
> > >>     <f6ad>   DW_AT_declaration : 1
> > >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> > >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> > >bpf_stats_enabled_mutex
> > >> [root@quaco perf]#
> > >>
> > >> I get what you have when I use elfutils' readelf:
> > >>
> > >> [root@quaco perf]# eu-readelf -winfo
> > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > >>              name                 (strp) "bpf_prog_active"
> > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > >> [root@quaco perf]#
> > >>
> > >> [root@quaco perf]# eu-readelf -winfo
> > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> > >\"bpf_prog_active\"
> > >>  [  f6a0]    variable             abbrev: 103
> > >>              name                 (strp) "bpf_prog_active"
> > >>              decl_file            (data1) bpf.h (11)
> > >>              decl_line            (data2) 1008
> > >>              decl_column          (data1) 1
> > >>              type                 (ref4) [    cf]
> > >>              external             (flag_present) yes
> > >>              declaration          (flag_present) yes
> > >>  [  f6ad]    variable             abbrev: 103
> > >>              name                 (strp) "bpf_stats_enabled_mutex"
> > >> [root@quaco perf]#
> > >>
> > >> And:
> > >>
> > >> [root@quaco perf]# eu-readelf -winfo
> > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> > >>  [ 1bdf5]    variable             abbrev: 212
> > >>              specification        (ref4) [  f6a0]
> > >>              decl_file            (data1) syscall.c (1)
> > >>              decl_line            (data1) 43
> > >>              location             (exprloc)
> > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > >> [root@quaco perf]#
> > >>
> > >
> > >In binutils readelf, there is a extra entry
> >
> > Not here, tomorrow I'll triple check.
> >
> > >
> > > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> > >    <1b24e>   DW_AT_specification: <0xf335>
> > >    <1b252>   DW_AT_decl_file   : 1
> > >    <1b253>   DW_AT_decl_line   : 43
> > >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > > (DW_OP_addr: 0)
> > >
> > >which points to
> > >
> > > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> > >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> > >bpf_prog_active
> > >
> > >It just doesn't have the string 'bpf_prog_active', annotating entry.
> > >So eu-readelf and binutils readelf have the same results.
> > >
> > >> > Note that second DW_TAG_variable entry contains specification that
> > >> > points to the first entry.
> > >>
> > >> So you are not considering the first when encoding since it is just a
> > >> DW_AT_declaration, considers the second, as it should be, and then
> > >needs
> > >> to go see its DW_AT_specification, right?
> > >>
> > >> Sounds correct, applying, will test further and then push out,
> > >>
> > >
> > >Yes, exactly. The var tags to be considered are those that either have
> > >DW_AT_specification or not have DW_AT_declaration. This makes sure
> > >btf_encoder works correctly on both old and new gcc.
> > >
> > >> Thanks,
> > >>
> > >> - Arnaldo
> > >
> > >Suggested by Yonghong, I tested this change on a larger set of
> > >compilers this time and works correctly. See below.
> > >
> > >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> > >have done that when sending out this patch. The credit goes to
> > >Yonghong.
> >
> > Sure, and I'll add your results with different computers, for the record.
> >
> > Thanks,
> >
> > - Arnaldo
> > >
> > >Thank you,
> > >Hao
> > >
> > >  clang 10:
> > >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> > >
> > >  clang 9:
> > >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> > >
> > >  gcc 10.2
> > >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> > >
> > >  gcc 9.3:
> > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > >
> > >  gcc 8
> > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > >
> > >  gcc 6.2
> > >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> > >
> > >  gcc 4.9
> > >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
> >
> > --
> > Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 15:47         ` Alexei Starovoitov
@ 2020-10-01 18:24           ` Arnaldo Carvalho de Melo
  2020-10-01 18:40             ` Hao Luo
  0 siblings, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-10-01 18:24 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Hao Luo, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo,
	Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, dwarves,
	Yonghong Song, bpf

Em Thu, Oct 01, 2020 at 08:47:51AM -0700, Alexei Starovoitov escreveu:
> Arnaldo,
> 
> ping.
> Is anything blocking this fix from merging?
> The kernel patches are stalled waiting on the pahole.

Applied locally, testing now, will push to the main branch ASAP.

- Arnaldo
 
> Thanks
> On Tue, Sep 29, 2020 at 11:52 PM Hao Luo <haoluo@google.com> wrote:
> >
> > Arnaldo,
> >
> > Is this patch ready to be merged into Pahole's master branch? Alexei
> > is testing the kernel patches that need this patch. Please let me know
> > if there is anything I can do to help merging.
> >
> > Thank you,
> > Hao
> >
> > On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
> > <arnaldo.melo@gmail.com> wrote:
> > >
> > >
> > >
> > > On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> > > >Arnaldo,
> > > >
> > > >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> > > ><acme@kernel.org> wrote:
> > > >>
> > > >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> > > >> > It is found on gcc 8.2 that global percpu variables generate the
> > > >> > following dwarf entry in the cu where the variable is defined[1].
> > > >> >
> > > >> > Take the global variable "bpf_prog_active" defined in
> > > >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> > > >> > has two dwarf entries for "bpf_prog_active".
> > > >> >
> > > >[...]
> > > >>
> > > >> Interesting, here I get, with binutils' readelf:
> > > >>
> > > >> [root@quaco perf]# readelf -wi
> > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > >bpf_prog_active
> > > >> [root@quaco perf]#
> > > >>
> > > >> Just one, as:
> > > >>
> > > >> [root@quaco perf]# readelf -wi
> > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> > > >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > >bpf_prog_active
> > > >>     <f6a5>   DW_AT_decl_file   : 11
> > > >>     <f6a6>   DW_AT_decl_line   : 1008
> > > >>     <f6a8>   DW_AT_decl_column : 1
> > > >>     <f6a9>   DW_AT_type        : <0xcf>
> > > >>     <f6ad>   DW_AT_external    : 1
> > > >>     <f6ad>   DW_AT_declaration : 1
> > > >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> > > >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> > > >bpf_stats_enabled_mutex
> > > >> [root@quaco perf]#
> > > >>
> > > >> I get what you have when I use elfutils' readelf:
> > > >>
> > > >> [root@quaco perf]# eu-readelf -winfo
> > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > >>              name                 (strp) "bpf_prog_active"
> > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > >> [root@quaco perf]#
> > > >>
> > > >> [root@quaco perf]# eu-readelf -winfo
> > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> > > >\"bpf_prog_active\"
> > > >>  [  f6a0]    variable             abbrev: 103
> > > >>              name                 (strp) "bpf_prog_active"
> > > >>              decl_file            (data1) bpf.h (11)
> > > >>              decl_line            (data2) 1008
> > > >>              decl_column          (data1) 1
> > > >>              type                 (ref4) [    cf]
> > > >>              external             (flag_present) yes
> > > >>              declaration          (flag_present) yes
> > > >>  [  f6ad]    variable             abbrev: 103
> > > >>              name                 (strp) "bpf_stats_enabled_mutex"
> > > >> [root@quaco perf]#
> > > >>
> > > >> And:
> > > >>
> > > >> [root@quaco perf]# eu-readelf -winfo
> > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> > > >>  [ 1bdf5]    variable             abbrev: 212
> > > >>              specification        (ref4) [  f6a0]
> > > >>              decl_file            (data1) syscall.c (1)
> > > >>              decl_line            (data1) 43
> > > >>              location             (exprloc)
> > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > >> [root@quaco perf]#
> > > >>
> > > >
> > > >In binutils readelf, there is a extra entry
> > >
> > > Not here, tomorrow I'll triple check.
> > >
> > > >
> > > > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> > > >    <1b24e>   DW_AT_specification: <0xf335>
> > > >    <1b252>   DW_AT_decl_file   : 1
> > > >    <1b253>   DW_AT_decl_line   : 43
> > > >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > > > (DW_OP_addr: 0)
> > > >
> > > >which points to
> > > >
> > > > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> > > >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> > > >bpf_prog_active
> > > >
> > > >It just doesn't have the string 'bpf_prog_active', annotating entry.
> > > >So eu-readelf and binutils readelf have the same results.
> > > >
> > > >> > Note that second DW_TAG_variable entry contains specification that
> > > >> > points to the first entry.
> > > >>
> > > >> So you are not considering the first when encoding since it is just a
> > > >> DW_AT_declaration, considers the second, as it should be, and then
> > > >needs
> > > >> to go see its DW_AT_specification, right?
> > > >>
> > > >> Sounds correct, applying, will test further and then push out,
> > > >>
> > > >
> > > >Yes, exactly. The var tags to be considered are those that either have
> > > >DW_AT_specification or not have DW_AT_declaration. This makes sure
> > > >btf_encoder works correctly on both old and new gcc.
> > > >
> > > >> Thanks,
> > > >>
> > > >> - Arnaldo
> > > >
> > > >Suggested by Yonghong, I tested this change on a larger set of
> > > >compilers this time and works correctly. See below.
> > > >
> > > >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> > > >have done that when sending out this patch. The credit goes to
> > > >Yonghong.
> > >
> > > Sure, and I'll add your results with different computers, for the record.
> > >
> > > Thanks,
> > >
> > > - Arnaldo
> > > >
> > > >Thank you,
> > > >Hao
> > > >
> > > >  clang 10:
> > > >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> > > >
> > > >  clang 9:
> > > >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> > > >
> > > >  gcc 10.2
> > > >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> > > >
> > > >  gcc 9.3:
> > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > >
> > > >  gcc 8
> > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > >
> > > >  gcc 6.2
> > > >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> > > >
> > > >  gcc 4.9
> > > >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
> > >
> > > --
> > > Sent from my Android device with K-9 Mail. Please excuse my brevity.

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 18:24           ` Arnaldo Carvalho de Melo
@ 2020-10-01 18:40             ` Hao Luo
  2020-10-01 20:27               ` Arnaldo Carvalho de Melo
  0 siblings, 1 reply; 8+ messages in thread
From: Hao Luo @ 2020-10-01 18:40 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alexei Starovoitov, Arnaldo Carvalho de Melo,
	Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

Thanks!

On Thu, Oct 1, 2020 at 11:24 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Oct 01, 2020 at 08:47:51AM -0700, Alexei Starovoitov escreveu:
> > Arnaldo,
> >
> > ping.
> > Is anything blocking this fix from merging?
> > The kernel patches are stalled waiting on the pahole.
>
> Applied locally, testing now, will push to the main branch ASAP.
>
> - Arnaldo
>
> > Thanks
> > On Tue, Sep 29, 2020 at 11:52 PM Hao Luo <haoluo@google.com> wrote:
> > >
> > > Arnaldo,
> > >
> > > Is this patch ready to be merged into Pahole's master branch? Alexei
> > > is testing the kernel patches that need this patch. Please let me know
> > > if there is anything I can do to help merging.
> > >
> > > Thank you,
> > > Hao
> > >
> > > On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
> > > <arnaldo.melo@gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> > > > >Arnaldo,
> > > > >
> > > > >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> > > > ><acme@kernel.org> wrote:
> > > > >>
> > > > >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> > > > >> > It is found on gcc 8.2 that global percpu variables generate the
> > > > >> > following dwarf entry in the cu where the variable is defined[1].
> > > > >> >
> > > > >> > Take the global variable "bpf_prog_active" defined in
> > > > >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> > > > >> > has two dwarf entries for "bpf_prog_active".
> > > > >> >
> > > > >[...]
> > > > >>
> > > > >> Interesting, here I get, with binutils' readelf:
> > > > >>
> > > > >> [root@quaco perf]# readelf -wi
> > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > >bpf_prog_active
> > > > >> [root@quaco perf]#
> > > > >>
> > > > >> Just one, as:
> > > > >>
> > > > >> [root@quaco perf]# readelf -wi
> > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> > > > >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > >bpf_prog_active
> > > > >>     <f6a5>   DW_AT_decl_file   : 11
> > > > >>     <f6a6>   DW_AT_decl_line   : 1008
> > > > >>     <f6a8>   DW_AT_decl_column : 1
> > > > >>     <f6a9>   DW_AT_type        : <0xcf>
> > > > >>     <f6ad>   DW_AT_external    : 1
> > > > >>     <f6ad>   DW_AT_declaration : 1
> > > > >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> > > > >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> > > > >bpf_stats_enabled_mutex
> > > > >> [root@quaco perf]#
> > > > >>
> > > > >> I get what you have when I use elfutils' readelf:
> > > > >>
> > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > >>              name                 (strp) "bpf_prog_active"
> > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > >> [root@quaco perf]#
> > > > >>
> > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> > > > >\"bpf_prog_active\"
> > > > >>  [  f6a0]    variable             abbrev: 103
> > > > >>              name                 (strp) "bpf_prog_active"
> > > > >>              decl_file            (data1) bpf.h (11)
> > > > >>              decl_line            (data2) 1008
> > > > >>              decl_column          (data1) 1
> > > > >>              type                 (ref4) [    cf]
> > > > >>              external             (flag_present) yes
> > > > >>              declaration          (flag_present) yes
> > > > >>  [  f6ad]    variable             abbrev: 103
> > > > >>              name                 (strp) "bpf_stats_enabled_mutex"
> > > > >> [root@quaco perf]#
> > > > >>
> > > > >> And:
> > > > >>
> > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> > > > >>  [ 1bdf5]    variable             abbrev: 212
> > > > >>              specification        (ref4) [  f6a0]
> > > > >>              decl_file            (data1) syscall.c (1)
> > > > >>              decl_line            (data1) 43
> > > > >>              location             (exprloc)
> > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > >> [root@quaco perf]#
> > > > >>
> > > > >
> > > > >In binutils readelf, there is a extra entry
> > > >
> > > > Not here, tomorrow I'll triple check.
> > > >
> > > > >
> > > > > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> > > > >    <1b24e>   DW_AT_specification: <0xf335>
> > > > >    <1b252>   DW_AT_decl_file   : 1
> > > > >    <1b253>   DW_AT_decl_line   : 43
> > > > >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > > > > (DW_OP_addr: 0)
> > > > >
> > > > >which points to
> > > > >
> > > > > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> > > > >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> > > > >bpf_prog_active
> > > > >
> > > > >It just doesn't have the string 'bpf_prog_active', annotating entry.
> > > > >So eu-readelf and binutils readelf have the same results.
> > > > >
> > > > >> > Note that second DW_TAG_variable entry contains specification that
> > > > >> > points to the first entry.
> > > > >>
> > > > >> So you are not considering the first when encoding since it is just a
> > > > >> DW_AT_declaration, considers the second, as it should be, and then
> > > > >needs
> > > > >> to go see its DW_AT_specification, right?
> > > > >>
> > > > >> Sounds correct, applying, will test further and then push out,
> > > > >>
> > > > >
> > > > >Yes, exactly. The var tags to be considered are those that either have
> > > > >DW_AT_specification or not have DW_AT_declaration. This makes sure
> > > > >btf_encoder works correctly on both old and new gcc.
> > > > >
> > > > >> Thanks,
> > > > >>
> > > > >> - Arnaldo
> > > > >
> > > > >Suggested by Yonghong, I tested this change on a larger set of
> > > > >compilers this time and works correctly. See below.
> > > > >
> > > > >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> > > > >have done that when sending out this patch. The credit goes to
> > > > >Yonghong.
> > > >
> > > > Sure, and I'll add your results with different computers, for the record.
> > > >
> > > > Thanks,
> > > >
> > > > - Arnaldo
> > > > >
> > > > >Thank you,
> > > > >Hao
> > > > >
> > > > >  clang 10:
> > > > >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> > > > >
> > > > >  clang 9:
> > > > >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> > > > >
> > > > >  gcc 10.2
> > > > >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> > > > >
> > > > >  gcc 9.3:
> > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > >
> > > > >  gcc 8
> > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > >
> > > > >  gcc 6.2
> > > > >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> > > > >
> > > > >  gcc 4.9
> > > > >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
> > > >
> > > > --
> > > > Sent from my Android device with K-9 Mail. Please excuse my brevity.
>
> --
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 18:40             ` Hao Luo
@ 2020-10-01 20:27               ` Arnaldo Carvalho de Melo
  2020-10-01 20:57                 ` Hao Luo
  0 siblings, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-10-01 20:27 UTC (permalink / raw)
  To: Hao Luo
  Cc: Alexei Starovoitov, Arnaldo Carvalho de Melo,
	Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

Em Thu, Oct 01, 2020 at 11:40:25AM -0700, Hao Luo escreveu:
> Thanks!

I must just apologise not having this in an officially released version
yet, getting constantly postponed due to bug reports about corner cases
and some features I got carried away working on, I'll fast pace a new
version to avoid getting in the way of the larger eBPF effort.

- Arnaldo
 
> On Thu, Oct 1, 2020 at 11:24 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Thu, Oct 01, 2020 at 08:47:51AM -0700, Alexei Starovoitov escreveu:
> > > Arnaldo,
> > >
> > > ping.
> > > Is anything blocking this fix from merging?
> > > The kernel patches are stalled waiting on the pahole.
> >
> > Applied locally, testing now, will push to the main branch ASAP.
> >
> > - Arnaldo
> >
> > > Thanks
> > > On Tue, Sep 29, 2020 at 11:52 PM Hao Luo <haoluo@google.com> wrote:
> > > >
> > > > Arnaldo,
> > > >
> > > > Is this patch ready to be merged into Pahole's master branch? Alexei
> > > > is testing the kernel patches that need this patch. Please let me know
> > > > if there is anything I can do to help merging.
> > > >
> > > > Thank you,
> > > > Hao
> > > >
> > > > On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
> > > > <arnaldo.melo@gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> > > > > >Arnaldo,
> > > > > >
> > > > > >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> > > > > ><acme@kernel.org> wrote:
> > > > > >>
> > > > > >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> > > > > >> > It is found on gcc 8.2 that global percpu variables generate the
> > > > > >> > following dwarf entry in the cu where the variable is defined[1].
> > > > > >> >
> > > > > >> > Take the global variable "bpf_prog_active" defined in
> > > > > >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> > > > > >> > has two dwarf entries for "bpf_prog_active".
> > > > > >> >
> > > > > >[...]
> > > > > >>
> > > > > >> Interesting, here I get, with binutils' readelf:
> > > > > >>
> > > > > >> [root@quaco perf]# readelf -wi
> > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > > >bpf_prog_active
> > > > > >> [root@quaco perf]#
> > > > > >>
> > > > > >> Just one, as:
> > > > > >>
> > > > > >> [root@quaco perf]# readelf -wi
> > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> > > > > >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> > > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > > >bpf_prog_active
> > > > > >>     <f6a5>   DW_AT_decl_file   : 11
> > > > > >>     <f6a6>   DW_AT_decl_line   : 1008
> > > > > >>     <f6a8>   DW_AT_decl_column : 1
> > > > > >>     <f6a9>   DW_AT_type        : <0xcf>
> > > > > >>     <f6ad>   DW_AT_external    : 1
> > > > > >>     <f6ad>   DW_AT_declaration : 1
> > > > > >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> > > > > >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> > > > > >bpf_stats_enabled_mutex
> > > > > >> [root@quaco perf]#
> > > > > >>
> > > > > >> I get what you have when I use elfutils' readelf:
> > > > > >>
> > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > > >>              name                 (strp) "bpf_prog_active"
> > > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > > >> [root@quaco perf]#
> > > > > >>
> > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> > > > > >\"bpf_prog_active\"
> > > > > >>  [  f6a0]    variable             abbrev: 103
> > > > > >>              name                 (strp) "bpf_prog_active"
> > > > > >>              decl_file            (data1) bpf.h (11)
> > > > > >>              decl_line            (data2) 1008
> > > > > >>              decl_column          (data1) 1
> > > > > >>              type                 (ref4) [    cf]
> > > > > >>              external             (flag_present) yes
> > > > > >>              declaration          (flag_present) yes
> > > > > >>  [  f6ad]    variable             abbrev: 103
> > > > > >>              name                 (strp) "bpf_stats_enabled_mutex"
> > > > > >> [root@quaco perf]#
> > > > > >>
> > > > > >> And:
> > > > > >>
> > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> > > > > >>  [ 1bdf5]    variable             abbrev: 212
> > > > > >>              specification        (ref4) [  f6a0]
> > > > > >>              decl_file            (data1) syscall.c (1)
> > > > > >>              decl_line            (data1) 43
> > > > > >>              location             (exprloc)
> > > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > > >> [root@quaco perf]#
> > > > > >>
> > > > > >
> > > > > >In binutils readelf, there is a extra entry
> > > > >
> > > > > Not here, tomorrow I'll triple check.
> > > > >
> > > > > >
> > > > > > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> > > > > >    <1b24e>   DW_AT_specification: <0xf335>
> > > > > >    <1b252>   DW_AT_decl_file   : 1
> > > > > >    <1b253>   DW_AT_decl_line   : 43
> > > > > >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > > > > > (DW_OP_addr: 0)
> > > > > >
> > > > > >which points to
> > > > > >
> > > > > > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> > > > > >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> > > > > >bpf_prog_active
> > > > > >
> > > > > >It just doesn't have the string 'bpf_prog_active', annotating entry.
> > > > > >So eu-readelf and binutils readelf have the same results.
> > > > > >
> > > > > >> > Note that second DW_TAG_variable entry contains specification that
> > > > > >> > points to the first entry.
> > > > > >>
> > > > > >> So you are not considering the first when encoding since it is just a
> > > > > >> DW_AT_declaration, considers the second, as it should be, and then
> > > > > >needs
> > > > > >> to go see its DW_AT_specification, right?
> > > > > >>
> > > > > >> Sounds correct, applying, will test further and then push out,
> > > > > >>
> > > > > >
> > > > > >Yes, exactly. The var tags to be considered are those that either have
> > > > > >DW_AT_specification or not have DW_AT_declaration. This makes sure
> > > > > >btf_encoder works correctly on both old and new gcc.
> > > > > >
> > > > > >> Thanks,
> > > > > >>
> > > > > >> - Arnaldo
> > > > > >
> > > > > >Suggested by Yonghong, I tested this change on a larger set of
> > > > > >compilers this time and works correctly. See below.
> > > > > >
> > > > > >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> > > > > >have done that when sending out this patch. The credit goes to
> > > > > >Yonghong.
> > > > >
> > > > > Sure, and I'll add your results with different computers, for the record.
> > > > >
> > > > > Thanks,
> > > > >
> > > > > - Arnaldo
> > > > > >
> > > > > >Thank you,
> > > > > >Hao
> > > > > >
> > > > > >  clang 10:
> > > > > >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> > > > > >
> > > > > >  clang 9:
> > > > > >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> > > > > >
> > > > > >  gcc 10.2
> > > > > >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> > > > > >
> > > > > >  gcc 9.3:
> > > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > > >
> > > > > >  gcc 8
> > > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > > >
> > > > > >  gcc 6.2
> > > > > >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> > > > > >
> > > > > >  gcc 4.9
> > > > > >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
> > > > >
> > > > > --
> > > > > Sent from my Android device with K-9 Mail. Please excuse my brevity.
> >
> > --
> >
> > - Arnaldo

-- 

- Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 20:27               ` Arnaldo Carvalho de Melo
@ 2020-10-01 20:57                 ` Hao Luo
  2020-10-01 21:07                   ` Alexei Starovoitov
  0 siblings, 1 reply; 8+ messages in thread
From: Hao Luo @ 2020-10-01 20:57 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Alexei Starovoitov, Arnaldo Carvalho de Melo,
	Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

Arnaldo, thanks for the update. In that case, I think on the kernel
side I need to skip encoding percpu vars for this pahole release, and
re-enable for the next pahole release. (assuming the flag for opt-out
is in this release). Alexei, do you have any better idea?

Hao

On Thu, Oct 1, 2020 at 1:27 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> Em Thu, Oct 01, 2020 at 11:40:25AM -0700, Hao Luo escreveu:
> > Thanks!
>
> I must just apologise not having this in an officially released version
> yet, getting constantly postponed due to bug reports about corner cases
> and some features I got carried away working on, I'll fast pace a new
> version to avoid getting in the way of the larger eBPF effort.
>
> - Arnaldo
>
> > On Thu, Oct 1, 2020 at 11:24 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Thu, Oct 01, 2020 at 08:47:51AM -0700, Alexei Starovoitov escreveu:
> > > > Arnaldo,
> > > >
> > > > ping.
> > > > Is anything blocking this fix from merging?
> > > > The kernel patches are stalled waiting on the pahole.
> > >
> > > Applied locally, testing now, will push to the main branch ASAP.
> > >
> > > - Arnaldo
> > >
> > > > Thanks
> > > > On Tue, Sep 29, 2020 at 11:52 PM Hao Luo <haoluo@google.com> wrote:
> > > > >
> > > > > Arnaldo,
> > > > >
> > > > > Is this patch ready to be merged into Pahole's master branch? Alexei
> > > > > is testing the kernel patches that need this patch. Please let me know
> > > > > if there is anything I can do to help merging.
> > > > >
> > > > > Thank you,
> > > > > Hao
> > > > >
> > > > > On Wed, Aug 26, 2020 at 6:56 PM Arnaldo Carvalho de Melo
> > > > > <arnaldo.melo@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On August 26, 2020 3:35:17 PM GMT-03:00, Hao Luo <haoluo@google.com> wrote:
> > > > > > >Arnaldo,
> > > > > > >
> > > > > > >On Wed, Aug 26, 2020 at 6:12 AM Arnaldo Carvalho de Melo
> > > > > > ><acme@kernel.org> wrote:
> > > > > > >>
> > > > > > >> Em Mon, Aug 24, 2020 at 05:45:23PM -0700, Hao Luo escreveu:
> > > > > > >> > It is found on gcc 8.2 that global percpu variables generate the
> > > > > > >> > following dwarf entry in the cu where the variable is defined[1].
> > > > > > >> >
> > > > > > >> > Take the global variable "bpf_prog_active" defined in
> > > > > > >> > kernel/bpf/syscall.c as an example. The debug info for syscall.c
> > > > > > >> > has two dwarf entries for "bpf_prog_active".
> > > > > > >> >
> > > > > > >[...]
> > > > > > >>
> > > > > > >> Interesting, here I get, with binutils' readelf:
> > > > > > >>
> > > > > > >> [root@quaco perf]# readelf -wi
> > > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > > > >bpf_prog_active
> > > > > > >> [root@quaco perf]#
> > > > > > >>
> > > > > > >> Just one, as:
> > > > > > >>
> > > > > > >> [root@quaco perf]# readelf -wi
> > > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active -B1 -A8
> > > > > > >>  <1><f6a0>: Abbrev Number: 103 (DW_TAG_variable)
> > > > > > >>     <f6a1>   DW_AT_name        : (indirect string, offset: 0xb70d):
> > > > > > >bpf_prog_active
> > > > > > >>     <f6a5>   DW_AT_decl_file   : 11
> > > > > > >>     <f6a6>   DW_AT_decl_line   : 1008
> > > > > > >>     <f6a8>   DW_AT_decl_column : 1
> > > > > > >>     <f6a9>   DW_AT_type        : <0xcf>
> > > > > > >>     <f6ad>   DW_AT_external    : 1
> > > > > > >>     <f6ad>   DW_AT_declaration : 1
> > > > > > >>  <1><f6ad>: Abbrev Number: 103 (DW_TAG_variable)
> > > > > > >>     <f6ae>   DW_AT_name        : (indirect string, offset: 0x3a5d):
> > > > > > >bpf_stats_enabled_mutex
> > > > > > >> [root@quaco perf]#
> > > > > > >>
> > > > > > >> I get what you have when I use elfutils' readelf:
> > > > > > >>
> > > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep bpf_prog_active
> > > > > > >>              name                 (strp) "bpf_prog_active"
> > > > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > > > >> [root@quaco perf]#
> > > > > > >>
> > > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B1 -A8
> > > > > > >\"bpf_prog_active\"
> > > > > > >>  [  f6a0]    variable             abbrev: 103
> > > > > > >>              name                 (strp) "bpf_prog_active"
> > > > > > >>              decl_file            (data1) bpf.h (11)
> > > > > > >>              decl_line            (data2) 1008
> > > > > > >>              decl_column          (data1) 1
> > > > > > >>              type                 (ref4) [    cf]
> > > > > > >>              external             (flag_present) yes
> > > > > > >>              declaration          (flag_present) yes
> > > > > > >>  [  f6ad]    variable             abbrev: 103
> > > > > > >>              name                 (strp) "bpf_stats_enabled_mutex"
> > > > > > >> [root@quaco perf]#
> > > > > > >>
> > > > > > >> And:
> > > > > > >>
> > > > > > >> [root@quaco perf]# eu-readelf -winfo
> > > > > > >../build/v5.8-rc5+/kernel/bpf/syscall.o | grep -B5 \<bpf_prog_active\>
> > > > > > >>  [ 1bdf5]    variable             abbrev: 212
> > > > > > >>              specification        (ref4) [  f6a0]
> > > > > > >>              decl_file            (data1) syscall.c (1)
> > > > > > >>              decl_line            (data1) 43
> > > > > > >>              location             (exprloc)
> > > > > > >>               [ 0] addr .data..percpu+0 <bpf_prog_active>
> > > > > > >> [root@quaco perf]#
> > > > > > >>
> > > > > > >
> > > > > > >In binutils readelf, there is a extra entry
> > > > > >
> > > > > > Not here, tomorrow I'll triple check.
> > > > > >
> > > > > > >
> > > > > > > <1><1b24c>: Abbrev Number: 195 (DW_TAG_variable)
> > > > > > >    <1b24e>   DW_AT_specification: <0xf335>
> > > > > > >    <1b252>   DW_AT_decl_file   : 1
> > > > > > >    <1b253>   DW_AT_decl_line   : 43
> > > > > > >    <1b254>   DW_AT_location    : 9 byte block: 3 0 0 0 0 0 0 0 0
> > > > > > > (DW_OP_addr: 0)
> > > > > > >
> > > > > > >which points to
> > > > > > >
> > > > > > > <1><f335>: Abbrev Number: 95 (DW_TAG_variable)
> > > > > > >    <f336>   DW_AT_name        : (indirect string, offset: 0xb37a):
> > > > > > >bpf_prog_active
> > > > > > >
> > > > > > >It just doesn't have the string 'bpf_prog_active', annotating entry.
> > > > > > >So eu-readelf and binutils readelf have the same results.
> > > > > > >
> > > > > > >> > Note that second DW_TAG_variable entry contains specification that
> > > > > > >> > points to the first entry.
> > > > > > >>
> > > > > > >> So you are not considering the first when encoding since it is just a
> > > > > > >> DW_AT_declaration, considers the second, as it should be, and then
> > > > > > >needs
> > > > > > >> to go see its DW_AT_specification, right?
> > > > > > >>
> > > > > > >> Sounds correct, applying, will test further and then push out,
> > > > > > >>
> > > > > > >
> > > > > > >Yes, exactly. The var tags to be considered are those that either have
> > > > > > >DW_AT_specification or not have DW_AT_declaration. This makes sure
> > > > > > >btf_encoder works correctly on both old and new gcc.
> > > > > > >
> > > > > > >> Thanks,
> > > > > > >>
> > > > > > >> - Arnaldo
> > > > > > >
> > > > > > >Suggested by Yonghong, I tested this change on a larger set of
> > > > > > >compilers this time and works correctly. See below.
> > > > > > >
> > > > > > >Could you also add 'Reported-by: Yonghong Song <yhs@fb.com>'? I should
> > > > > > >have done that when sending out this patch. The credit goes to
> > > > > > >Yonghong.
> > > > > >
> > > > > > Sure, and I'll add your results with different computers, for the record.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > - Arnaldo
> > > > > > >
> > > > > > >Thank you,
> > > > > > >Hao
> > > > > > >
> > > > > > >  clang 10:
> > > > > > >  [67] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [20168] VAR 'bpf_prog_active' type_id=67, linkage=global-alloc
> > > > > > >
> > > > > > >  clang 9:
> > > > > > >  [64] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [19789] VAR 'bpf_prog_active' type_id=64, linkage=global-alloc
> > > > > > >
> > > > > > >  gcc 10.2
> > > > > > >  [18] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [20319] VAR 'bpf_prog_active' type_id=18, linkage=global-alloc
> > > > > > >
> > > > > > >  gcc 9.3:
> > > > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [21085] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > > > >
> > > > > > >  gcc 8
> > > > > > >  [21] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [21084] VAR 'bpf_prog_active' type_id=21, linkage=global-alloc
> > > > > > >
> > > > > > >  gcc 6.2
> > > > > > >  [22] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [21083] VAR 'bpf_prog_active' type_id=22, linkage=global-alloc
> > > > > > >
> > > > > > >  gcc 4.9
> > > > > > >  [17] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> > > > > > >  [20410] VAR 'bpf_prog_active' type_id=17, linkage=global-alloc
> > > > > >
> > > > > > --
> > > > > > Sent from my Android device with K-9 Mail. Please excuse my brevity.
> > >
> > > --
> > >
> > > - Arnaldo
>
> --
>
> - Arnaldo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 20:57                 ` Hao Luo
@ 2020-10-01 21:07                   ` Alexei Starovoitov
  2020-10-01 21:31                     ` Hao Luo
  0 siblings, 1 reply; 8+ messages in thread
From: Alexei Starovoitov @ 2020-10-01 21:07 UTC (permalink / raw)
  To: Hao Luo
  Cc: Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo,
	Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

On Thu, Oct 1, 2020 at 1:57 PM Hao Luo <haoluo@google.com> wrote:
>
> Arnaldo, thanks for the update. In that case, I think on the kernel
> side I need to skip encoding percpu vars for this pahole release, and
> re-enable for the next pahole release. (assuming the flag for opt-out
> is in this release). Alexei, do you have any better idea?

I'm not following. Let's get this fix landed in pahole and
release new 1.18 with it.
The opt-out flag is orthogonal. I can be done in 1.19 or whenever.
With your kernel patches the kernel will reject percpu vars when pahole
is too old, because they will not be found in vmlinux btf,
so I don't see any compatibility issues.
There is no need to bump the required min version of pahole in
scripts/link-vmlinux.sh.
It can stay as v1.16. We only need clean verifier message that percpu BTF
is not found and the kernel needs to be rebuilt with pahole 1.18.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
  2020-10-01 21:07                   ` Alexei Starovoitov
@ 2020-10-01 21:31                     ` Hao Luo
  0 siblings, 0 replies; 8+ messages in thread
From: Hao Luo @ 2020-10-01 21:31 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo,
	Arnaldo Carvalho de Melo, Andrii Nakryiko, Daniel Borkmann,
	Martin KaFai Lau, dwarves, Yonghong Song, bpf

On Thu, Oct 1, 2020 at 2:07 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 1:57 PM Hao Luo <haoluo@google.com> wrote:
> >
> > Arnaldo, thanks for the update. In that case, I think on the kernel
> > side I need to skip encoding percpu vars for this pahole release, and
> > re-enable for the next pahole release. (assuming the flag for opt-out
> > is in this release). Alexei, do you have any better idea?
>
> I'm not following. Let's get this fix landed in pahole and
> release new 1.18 with it.

Sorry, I misunderstood Arnaldo. I thought what he was saying was, not
having this fix in an officially released version (I took this as the
new 1.18) and will get 1.19 quicker. It would be ideal if we can have
this in 1.18. Sorry for my bad reading.

> The opt-out flag is orthogonal. I can be done in 1.19 or whenever.
> With your kernel patches the kernel will reject percpu vars when pahole
> is too old, because they will not be found in vmlinux btf,
> so I don't see any compatibility issues.
> There is no need to bump the required min version of pahole in
> scripts/link-vmlinux.sh.
> It can stay as v1.16. We only need clean verifier message that percpu BTF
> is not found and the kernel needs to be rebuilt with pahole 1.18.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-10-01 21:32 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200825004523.1353133-1-haoluo@google.com>
     [not found] ` <20200826131143.GF1059382@kernel.org>
     [not found]   ` <CA+khW7jf7Z=sMC1u5eyn6XOZDTFJaNjV-D0ogvQSyUGSKjC3LQ@mail.gmail.com>
     [not found]     ` <DEC4CC81-88CE-4476-A631-2BBB6E922F5C@gmail.com>
2020-09-30  6:52       ` [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification Hao Luo
2020-10-01 15:47         ` Alexei Starovoitov
2020-10-01 18:24           ` Arnaldo Carvalho de Melo
2020-10-01 18:40             ` Hao Luo
2020-10-01 20:27               ` Arnaldo Carvalho de Melo
2020-10-01 20:57                 ` Hao Luo
2020-10-01 21:07                   ` Alexei Starovoitov
2020-10-01 21:31                     ` Hao Luo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).