Dwarves Archive on lore.kernel.org
 help / color / Atom feed
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Hao Luo <haoluo@google.com>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>,
	dwarves@vger.kernel.org, Yonghong Song <yhs@fb.com>,
	bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH v1] btf_encoder: Handle DW_TAG_variable that has DW_AT_specification
Date: Thu, 1 Oct 2020 17:27:29 -0300
Message-ID: <20201001202729.GA105734@kernel.org> (raw)
In-Reply-To: <CA+khW7iSd4EX0EdoQ0+FvnGg5CKai+TLsa4xbDUPA8tbiu3LZw@mail.gmail.com>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [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       ` 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 [this message]
2020-10-01 20:57                 ` Hao Luo
2020-10-01 21:07                   ` Alexei Starovoitov
2020-10-01 21:31                     ` Hao Luo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201001202729.GA105734@kernel.org \
    --to=acme@kernel.org \
    --cc=acme@redhat.com \
    --cc=alexei.starovoitov@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=arnaldo.melo@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=dwarves@vger.kernel.org \
    --cc=haoluo@google.com \
    --cc=kafai@fb.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Dwarves Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dwarves/0 dwarves/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dwarves dwarves/ https://lore.kernel.org/dwarves \
		dwarves@vger.kernel.org
	public-inbox-index dwarves

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dwarves


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git