[PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[Jiri Olsa]

hi,
recent btf encoder's changes brakes BTF data for some gcc
versions. The problem is that some functions can appear
in dwarf data in some instances without arguments, while
they are defined with some.

v3 changes:
  - move 'generated' flag set out of should_generate_function
  - rename should_generate_function to find_function
  - added ack

v2 changes:
  - drop patch 3 logic and just change conditions
    based on Andrii's suggestion
  - drop patch 2
  - add ack for patch 1

thanks,
jirka


---
Jiri Olsa (2):
      btf_encoder: Generate also .init functions
      btf_encoder: Fix function generation

 btf_encoder.c | 86 +++++++++++++++++++++-----------------------------------------------------------------
 1 file changed, 21 insertions(+), 65 deletions(-)

[PATCH 1/2] btf_encoder: Generate also .init functions

[Jiri Olsa]

Currently we skip functions under .init* sections, Removing the .init*
section check, BTF now contains also functions from .init* sections.

Andrii's explanation from email:

> ...                  I think we should just drop the __init check and
> include all the __init functions into BTF. There could be cases where
> we'd need to attach BPF programs to __init functions (e.g., bpf_lsm
> security cases), so having BTFs for those FUNCs are necessary as well.
> Ftrace currently disallows that, but it's only because no user-space
> application has a way to attach probes early enough. This might change
> in the future, so there is no need to invent special mechanisms now
> for bpf_iter function preservation. Let's just include all __init
> functions in BTF.

It's over ~2000 functions on my .config:

   $ bpftool btf dump file ./vmlinux | grep 'FUNC ' | wc -l
   41505
   $ bpftool btf dump file /sys/kernel/btf/vmlinux | grep 'FUNC ' | wc -l
   39256

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 43 ++-----------------------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index 9b93e9963727..d531651b1e9e 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -29,10 +29,6 @@
 struct funcs_layout {
 	unsigned long mcount_start;
 	unsigned long mcount_stop;
-	unsigned long init_begin;
-	unsigned long init_end;
-	unsigned long init_bpf_begin;
-	unsigned long init_bpf_end;
 	unsigned long mcount_sec_idx;
 };
 
@@ -104,16 +100,6 @@ static int addrs_cmp(const void *_a, const void *_b)
 	return *a < *b ? -1 : 1;
 }
 
-static bool is_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_begin && addr < fl->init_end;
-}
-
-static bool is_bpf_init(struct funcs_layout *fl, unsigned long addr)
-{
-	return addr >= fl->init_bpf_begin && addr < fl->init_bpf_end;
-}
-
 static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 {
 	unsigned long *addrs, count, offset, i;
@@ -155,18 +141,11 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 
 	/*
 	 * Let's got through all collected functions and filter
-	 * out those that are not in ftrace and init code.
+	 * out those that are not in ftrace.
 	 */
 	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(fl, func->addr) && !is_bpf_init(fl, func->addr))
-			continue;
-
 		/* Make sure function is within ftrace addresses. */
 		if (bsearch(&func->addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
 			/*
@@ -493,29 +472,11 @@ static void collect_symbol(GElf_Sym *sym, struct funcs_layout *fl)
 	if (!fl->mcount_stop &&
 	    !strcmp("__stop_mcount_loc", elf_sym__name(sym, btfe->symtab)))
 		fl->mcount_stop = sym->st_value;
-
-	if (!fl->init_begin &&
-	    !strcmp("__init_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_begin = sym->st_value;
-
-	if (!fl->init_end &&
-	    !strcmp("__init_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_end = sym->st_value;
-
-	if (!fl->init_bpf_begin &&
-	    !strcmp("__init_bpf_preserve_type_begin", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_begin = sym->st_value;
-
-	if (!fl->init_bpf_end &&
-	    !strcmp("__init_bpf_preserve_type_end", elf_sym__name(sym, btfe->symtab)))
-		fl->init_bpf_end = sym->st_value;
 }
 
 static int has_all_symbols(struct funcs_layout *fl)
 {
-	return fl->mcount_start && fl->mcount_stop &&
-	       fl->init_begin && fl->init_end &&
-	       fl->init_bpf_begin && fl->init_bpf_end;
+	return fl->mcount_start && fl->mcount_stop;
 }
 
 static int collect_symbols(struct btf_elf *btfe, bool collect_percpu_vars)
-- 
2.26.2

[PATCH 2/2] btf_encoder: Fix function generation

[Jiri Olsa]

Current conditions for picking up function records break
BTF data on some gcc versions.

Some function records can appear with no arguments but with
declaration tag set, so moving the 'fn->declaration' in front
of other checks.

Then checking if argument names are present and finally checking
ftrace filter if it's present. If ftrace filter is not available,
using the external tag to filter out non external functions.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 btf_encoder.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/btf_encoder.c b/btf_encoder.c
index d531651b1e9e..f3f6291391ee 100644
--- a/btf_encoder.c
+++ b/btf_encoder.c
@@ -164,18 +164,13 @@ static int filter_functions(struct btf_elf *btfe, struct funcs_layout *fl)
 	return 0;
 }
 
-static bool should_generate_function(const struct btf_elf *btfe, const char *name)
+static struct elf_function *find_function(const struct btf_elf *btfe,
+					  const char *name)
 {
-	struct elf_function *p;
 	struct elf_function key = { .name = name };
 
-	p = bsearch(&key, functions, functions_cnt,
-		    sizeof(functions[0]), functions_cmp);
-	if (!p || p->generated)
-		return false;
-
-	p->generated = true;
-	return true;
+	return bsearch(&key, functions, functions_cnt, sizeof(functions[0]),
+		       functions_cmp);
 }
 
 static bool btf_name_char_ok(char c, bool first)
@@ -612,25 +607,25 @@ int cu__encode_btf(struct cu *cu, int verbose, bool force,
 		const char *name;
 
 		/*
-		 * The functions_cnt != 0 means we parsed all necessary
-		 * kernel symbols and we are using ftrace location filter
-		 * for functions. If it's not available keep the current
-		 * dwarf declaration check.
+		 * Skip functions that:
+		 *   - are marked as declarations
+		 *   - do not have full argument names
+		 *   - are not in ftrace list (if it's available)
+		 *   - are not external (in case ftrace filter is not available)
 		 */
+		if (fn->declaration)
+			continue;
+		if (!has_arg_names(cu, &fn->proto))
+			continue;
 		if (functions_cnt) {
-			/*
-			 * We check following conditions:
-			 *   - argument names are defined
-			 *   - there's symbol and address defined for the function
-			 *   - function address belongs to ftrace locations
-			 *   - function is generated only once
-			 */
-			if (!has_arg_names(cu, &fn->proto))
-				continue;
-			if (!should_generate_function(btfe, function__name(fn, cu)))
+			struct elf_function *func;
+
+			func = find_function(btfe, function__name(fn, cu));
+			if (!func || func->generated)
 				continue;
+			func->generated = true;
 		} else {
-			if (fn->declaration || !fn->external)
+			if (!fn->external)
 				continue;
 		}
 
-- 
2.26.2

Re: [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[Andrii Nakryiko]

On Sat, Nov 14, 2020 at 2:39 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> hi,
> recent btf encoder's changes brakes BTF data for some gcc
> versions. The problem is that some functions can appear
> in dwarf data in some instances without arguments, while
> they are defined with some.

Hey Jiri,

So this approach with __start_mcount_loc/__stop_mcount_loc works for
vmlinux only, but it doesn't work for kernel modules. For kernel
modules there is a dedicated "__mcount_loc" section, but no
__start/__stop symbols. I'm working around for now by making sure
functions that I need are global, but it would be nice to have this
working for modules out of the box as well.

If you get a chance to fix this soon, that would be great. If not,
I'll try to get to this ASAP as well, because it would be nice to have
this in the same version of pahole that got static function BTFs for
vmlinux (if Arnaldo doesn't mind, of course).

>
> v3 changes:
>   - move 'generated' flag set out of should_generate_function
>   - rename should_generate_function to find_function
>   - added ack
>
> v2 changes:
>   - drop patch 3 logic and just change conditions
>     based on Andrii's suggestion
>   - drop patch 2
>   - add ack for patch 1
>
> thanks,
> jirka
>
>
> ---
> Jiri Olsa (2):
>       btf_encoder: Generate also .init functions
>       btf_encoder: Fix function generation
>
>  btf_encoder.c | 86 +++++++++++++++++++++-----------------------------------------------------------------
>  1 file changed, 21 insertions(+), 65 deletions(-)
>

Re: [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[Jiri Olsa]

On Fri, Nov 20, 2020 at 05:13:24PM -0800, Andrii Nakryiko wrote:
> On Sat, Nov 14, 2020 at 2:39 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > hi,
> > recent btf encoder's changes brakes BTF data for some gcc
> > versions. The problem is that some functions can appear
> > in dwarf data in some instances without arguments, while
> > they are defined with some.
> 
> Hey Jiri,
> 
> So this approach with __start_mcount_loc/__stop_mcount_loc works for
> vmlinux only, but it doesn't work for kernel modules. For kernel
> modules there is a dedicated "__mcount_loc" section, but no
> __start/__stop symbols. I'm working around for now by making sure
> functions that I need are global, but it would be nice to have this
> working for modules out of the box as well.

hi,
I checked and it's bit more tricky than with vmlinux,
addresses are in __mcount_loc, but it's all zeros and
it gets filled after via relocation from .rela__mcount_loc

I think we could do relocation of __mcount_loc section
with zero base and get all base addresses.. and then
continue from there with current code checks

I'll check on it tomorrow

> 
> If you get a chance to fix this soon, that would be great. If not,
> I'll try to get to this ASAP as well, because it would be nice to have
> this in the same version of pahole that got static function BTFs for
> vmlinux (if Arnaldo doesn't mind, of course).

we're eagerly expecting the new pahole with the DWARF bug
workaround, so we asked Arnaldo to release soon, how big
problem is it for you if the modules fix is in the next one?

thanks,
jirka

> 
> >
> > v3 changes:
> >   - move 'generated' flag set out of should_generate_function
> >   - rename should_generate_function to find_function
> >   - added ack
> >
> > v2 changes:
> >   - drop patch 3 logic and just change conditions
> >     based on Andrii's suggestion
> >   - drop patch 2
> >   - add ack for patch 1
> >
> > thanks,
> > jirka
> >
> >
> > ---
> > Jiri Olsa (2):
> >       btf_encoder: Generate also .init functions
> >       btf_encoder: Fix function generation
> >
> >  btf_encoder.c | 86 +++++++++++++++++++++-----------------------------------------------------------------
> >  1 file changed, 21 insertions(+), 65 deletions(-)
> >
> 

Re: [PATCHv3 0/2] btf_encoder: Fix functions BTF data generation

[Andrii Nakryiko]

On Sun, Nov 22, 2020 at 2:56 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Nov 20, 2020 at 05:13:24PM -0800, Andrii Nakryiko wrote:
> > On Sat, Nov 14, 2020 at 2:39 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > hi,
> > > recent btf encoder's changes brakes BTF data for some gcc
> > > versions. The problem is that some functions can appear
> > > in dwarf data in some instances without arguments, while
> > > they are defined with some.
> >
> > Hey Jiri,
> >
> > So this approach with __start_mcount_loc/__stop_mcount_loc works for
> > vmlinux only, but it doesn't work for kernel modules. For kernel
> > modules there is a dedicated "__mcount_loc" section, but no
> > __start/__stop symbols. I'm working around for now by making sure
> > functions that I need are global, but it would be nice to have this
> > working for modules out of the box as well.
>
> hi,
> I checked and it's bit more tricky than with vmlinux,
> addresses are in __mcount_loc, but it's all zeros and
> it gets filled after via relocation from .rela__mcount_loc
>
> I think we could do relocation of __mcount_loc section
> with zero base and get all base addresses.. and then
> continue from there with current code checks
>
> I'll check on it tomorrow

Thanks!

>
> >
> > If you get a chance to fix this soon, that would be great. If not,
> > I'll try to get to this ASAP as well, because it would be nice to have
> > this in the same version of pahole that got static function BTFs for
> > vmlinux (if Arnaldo doesn't mind, of course).
>
> we're eagerly expecting the new pahole with the DWARF bug
> workaround, so we asked Arnaldo to release soon, how big
> problem is it for you if the modules fix is in the next one?
>

Not a problem, I just hate remembering all the versions of all the
binaries/libraries/compilers and what each version added. 1.19 had a
chance to be the version which makes fentry/fexit work for all cases,
but I guess it won't happen :) No big deal.

> thanks,
> jirka
>
> >
> > >
> > > v3 changes:
> > >   - move 'generated' flag set out of should_generate_function
> > >   - rename should_generate_function to find_function
> > >   - added ack
> > >
> > > v2 changes:
> > >   - drop patch 3 logic and just change conditions
> > >     based on Andrii's suggestion
> > >   - drop patch 2
> > >   - add ack for patch 1
> > >
> > > thanks,
> > > jirka
> > >
> > >
> > > ---
> > > Jiri Olsa (2):
> > >       btf_encoder: Generate also .init functions
> > >       btf_encoder: Fix function generation
> > >
> > >  btf_encoder.c | 86 +++++++++++++++++++++-----------------------------------------------------------------
> > >  1 file changed, 21 insertions(+), 65 deletions(-)
> > >
> >
>