Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf

From: Jiri Olsa <jolsa@redhat.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Jiri Olsa <jolsa@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	dwarves@vger.kernel.org, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andriin@fb.com>, Yonghong Song <yhs@fb.com>,
	Hao Luo <haoluo@google.com>, "Frank Ch. Eigler" <fche@redhat.com>,
	Mark Wielaard <mjw@redhat.com>
Subject: Re: [PATCH 3/3] btf_encoder: Change functions check due to broken dwarf
Date: Thu, 5 Nov 2020 23:56:17 +0100	[thread overview]
Message-ID: <20201105225617.GB4112111@krava> (raw)
In-Reply-To: <CAEf4BzZaUyY0TA_Gq559ojEeT2mHtdc3aUbvB9Q_4u0pZ+WiWQ@mail.gmail.com>

On Thu, Nov 05, 2020 at 11:52:35AM -0800, Andrii Nakryiko wrote:

SNIP

> > +        * Let's got through all collected functions and filter
> > +        * out those that are not in ftrace and init code.
> > +        */
> > +       for (i = 0; i < functions_cnt; i++) {
> > +               struct elf_function *func = &functions[i];
> > +
> > +               /*
> > +                * Do not enable .init section functions,
> > +                * but keep .init.bpf.preserve_type functions.
> > +                */
> > +               if (is_init(ms, func->addr) && !is_bpf_init(ms, func->addr))
> > +                       continue;
> > +
> > +               /* Make sure function is within ftrace addresses. */
> > +               if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > +                       /*
> > +                        * We iterate over sorted array, so we can easily skip
> > +                        * not valid item and move following valid field into
> > +                        * its place, and still keep the 'new' array sorted.
> > +                        */
> > +                       if (i != functions_valid)
> > +                               functions[functions_valid] = functions[i];
> > +                       functions_valid++;
> > +               }
> > +       }
> 
> can we re-assign function_cnt = functions_valid here? and
> functions_valid could be just a local temporary variable?

good idea, should be simpler.. will change

and ack to all naming changes above ;-)

thanks,
jirka

> 
> > +
> > +       free(addrs);
> > +       return 0;
> > +}
> > +
> 
> [...]
> 