Dwarves Archive on lore.kernel.org
 help / color / Atom feed
From: Hao Luo <haoluo-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
To: Arnaldo Carvalho de Melo <acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Andrii Nakryiko
	<andrii.nakryiko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Arnaldo Carvalho de Melo
	<arnaldo.melo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Alexei Starovoitov
	<alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>,
	Oleg Rombakh <olegrom-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
	Martin Lau <kafai-b10kYP2dOMg@public.gmane.org>,
	dwarves-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v4] btf_encoder: Teach pahole to store percpu variables in vmlinux BTF.
Date: Fri, 3 Jul 2020 01:13:07 -0700
Message-ID: <CA+khW7iwOnVacJ4XCNU=0RtT-2T=vX4usLh7LDayOSH97=j1KA@mail.gmail.com> (raw)
In-Reply-To: <20200630115927.GL29008-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Some update on my side.

I am sending a v5 based on Andrii's suggestion on using elf_sym__name
and elf_sym__size. It seems my local build tool has encoded all the
symbols into DWARF, therefore, I don't see those "(anon)" variables in
either v4 or v5, as seen by Andrii and Arnaldo. But I did come up with
a potential concern about the variable's type while writing v5. Right
now, the btf_id of the variable's type is retrieved as

type = var->ip.tag.type + type_id_off;

If I understand correctly, this is obtained from DWARF. Since symtab
doesn't store the symbol's type (please correct me if I'm wrong), even
if we can get the variable's name and size using elf_sym__name and
elf_sym__size, we have no way to get the type. I am afraid we may get
an incorrect type id using var->ip.tag.type.

So, Andrii, could you please verify whether the encoded VAR's types
are still correct for those 'anon' variables? I can't verify as my
build tool doesn't generate such variables. If the encoded type ids
are still correct, then using sym does do a better job than using
DWARF.

I'm also leaving the basic validations on variable's name and size in
v5. I think they are needed. It's better having less variables than a
malformed btf if things go awry.

Hao

On Tue, Jun 30, 2020 at 4:59 AM Arnaldo Carvalho de Melo
<acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Em Mon, Jun 29, 2020 at 01:20:02PM -0700, Andrii Nakryiko escreveu:
> > On Fri, Jun 19, 2020 at 12:58 PM Andrii Nakryiko <andrii.nakryiko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > > On Thu, Jun 18, 2020 at 12:49 AM Hao Luo <haoluo-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> > > > +               if (!addr)
> > > > +                       continue;
> > > > +               var = hashaddr__find_variable(hash_addr, addr);
> > > > +               if (var == NULL)
> > > > +                       continue;
> > > > +
> > > > +               /*
> > > > +                * Valididate the variable's name, the same check is also
> > > > +                * performed in bpf verifier.
> > > > +                */
> > > > +               name = variable__name(var, cu);
> > > > +               if (!var->name || !btf_name_valid(name, true)) {
> > >
> > > It might be ok to filter them out, but I'd rather try to first
> > > understand why do we get those invalid names in the first place.
> > > Otherwise we might be just papering over the real bug in how we
> > > process symbols/variables in DWARF?
> >
> > Ok, so I played with it a bit locally. Those variables with empty name
> > or 0 size seem to be due to some bugs in variable__name() and
> > variable__type_size() implementations. If you stick to ELF symbol's
> > elf_sym__name() and elf_sym__size(), you'll get valid sizes.
> >
> > Arnaldo, could you please take a look at why variable__xxx() impls
> > don't work for some variables? I can repro it locally, e.g.:
>
> I'm looking at it, I think this is that maybe not all variables that are
> in the elf symbol table are encoded in DWARF (maybe some declared in asm
> files?) and thus we will not find its name in the .debug_str.
>
> ctf_loader.c, that was used as the model for btf_loader.c has this:
>
> static const char *ctf__variable_name(const struct variable *var,
>                                       const struct cu *cu)
> {
>         struct ctf *ctf = cu->priv;
>
>         return ctf->symtab->symstrs->d_buf + var->name;
> }
>
> struct debug_fmt_ops ctf__ops = {
>         .name           = "ctf",
>         .function__name = ctf__function_name,
>         .load_file      = ctf__load_file,
>         .variable__name = ctf__variable_name,
>         .strings__ptr   = ctf__strings_ptr,
>         .cu__delete     = ctf__cu_delete,
> };
>
> And then
>
> const char *variable__name(const struct variable *var, const struct cu *cu)
> {
>         if (cu->dfops && cu->dfops->variable__name)
>                 return cu->dfops->variable__name(var, cu);
>         return s(cu, var->name);
> }
>
> I.e. btf_loader.c and dwarf_loader.c are using the fallback return that
> assumes all strings are in a single table, the CTF one gets variable
> names from the ELF symtab, as you seem to be doing by using
> elf_sym__name() directly.
>
> I'm reading Hao's patch to see how the variables are being created as
> they are loaded from DWARF, to check this hunch.
>
> - Arnaldo
>
> > ELF SYM kstat == <empty>, SZ 48 == 0
> > invalid type size for variable '(null)'.
> > invalid variable name '(null)'.
> >
> > kstat and 48 come from ELF symbol, <empty> and 0 are from
> > variabel__xxx(). And that happens for quite a few variables, actually.
> >
> > With those fixes, my kernel can be successfully BTF-generated. The
> > number of per-cpu variables jump from 213 with all this filtering to
> > 287 proper variables while using elf_sym__xxx() functions.
> >
> > >
> > > > +                       if (verbose)
> > > > +                               printf("invalid variable name '%s'.\n", name);
> > > > +                       continue;
> > > > +               }
> > > > +
> > > > +               /*
> > > > +                * Validate the variable's type, the same check is also
> > > > +                * performed in bpf verifier.
> > > > +                */
> > > > +               type = var->ip.tag.type + type_id_off;
> > > > +               if (type == 0) {
> > > > +                       if (verbose)
> > > > +                               printf("invalid type for variable '%s'.\n", name);
> > > > +                       continue;
> > >
> > > Again, there might be cases which we want to ignore, but we need to
> > > understand what and why those cases are?
> > >
> > > > +               }
> > > > +
> > > > +               /*
> > > > +                * Validate the size of the variable's type, the same check is
> > > > +                * also performed in bpf verifier.
> > > > +                */
> > > > +               size = variable__type_size(var, cu);
> > > > +               if (size == 0) {
> > > > +                       if (verbose)
> > > > +                               printf("invalid type size for variable '%s'.\n", name);
> > > > +                       continue;
> > > > +               }
> > > > +
> > >
> > > The goal here is not to satisfy the verifier, but to generate BTF from
> > > DWARF faithfully. Let's not just ignore "inconvenient" cases and
> > > instead investigate why such cases happen.
> > >
> > > > +               if (verbose)
> > > > +                       printf("symbol '%s' of address 0x%lx encoded\n",
> > > > +                              elf_sym__name(&sym, btfe->symtab), addr);
> > > > +
> > > > +               /* add a BTF_KIND_VAR in btfe->types */
> > > > +               linkage = var->external ? BTF_VAR_GLOBAL_ALLOCATED : BTF_VAR_STATIC;
> > > > +               btf_var_id = btf_elf__add_var_type(btfe, type, var->name, linkage);
> > > > +               if (btf_var_id < 0) {
> > > > +                       err = -1;
> > > > +                       printf("error: failed to encode variable '%s'\n", name);
> > > > +                       break;
> > > > +               }
> > > > +
> > > > +               /*
> > > > +                * add a BTF_VAR_SECINFO in btfe->percpu_secinfo, which will be added into
> > > > +                * btfe->types later when we add BTF_VAR_DATASEC.
> > > > +                */
> > > > +               type = btf_var_id;
> > > > +               offset = addr - btfe->percpu_base_addr;
> > > > +               btf_var_secinfo_id = btf_elf__add_var_secinfo(&btfe->percpu_secinfo,
> > > > +                                                             type, offset, size);
> > > > +               if (btf_var_secinfo_id < 0) {
> > > > +                       err = -1;
> > > > +                       printf("error: failed to encode var secinfo '%s'\n", name);
> > > > +                       break;
> > > > +               }
> > > > +       }
> > > > +
> > > >  out:
> > > >         if (err)
> > > >                 btf_elf__delete(btfe);
> > >
> > > [...]
>
> --
>
> - Arnaldo

      parent reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18  7:48 Hao Luo
     [not found] ` <20200618074853.133675-1-haoluo-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
2020-06-19 19:58   ` Andrii Nakryiko
     [not found]     ` <CAEf4BzbE2K7uyJ-2d3N+NLrV5OKS0HMm5vKuxMz5bfZkE_ANzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-06-19 20:30       ` Hao Luo
     [not found]         ` <CA+khW7j3AoW8q+Q30RXG-psw4imkBxNqffObnu2dr2=K=oVBCg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-06-29 20:25           ` Andrii Nakryiko
     [not found]             ` <CAEf4Bza20kx3d+84Fg+UKXFYb2vforj55g0zO92Hpz+cM49LQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-06-29 21:13               ` Hao Luo
     [not found]                 ` <CA+khW7hqX9fJd2G8-Kze9pMR4hUKV5ROxn1vKYy82+PCzDeCDQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-06-30  1:02                   ` Arnaldo Carvalho de Melo
2020-06-29 20:20       ` Andrii Nakryiko
     [not found]         ` <CAEf4Bzbiu8G5UK9TDBRFYGwVk4A4QWoLurZsFKhs439gBXaDCg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-06-30 11:59           ` Arnaldo Carvalho de Melo
     [not found]             ` <20200630115927.GL29008-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2020-07-03  8:13               ` Hao Luo [this message]

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='CA+khW7iwOnVacJ4XCNU=0RtT-2T=vX4usLh7LDayOSH97=j1KA@mail.gmail.com' \
    --to=haoluo-hpiqsd4aklfqt0dzr+alfa@public.gmane.org \
    --cc=acme-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=andrii.nakryiko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=arnaldo.melo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org \
    --cc=dwarves-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=kafai-b10kYP2dOMg@public.gmane.org \
    --cc=olegrom-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org \
    /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