* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-13 16:46 [PATCHv2] btf_encoder: Match ftrace addresses within elf functions Jiri Olsa
@ 2021-02-13 19:11 ` Sedat Dilek
2021-02-17 12:44 ` Arnaldo Carvalho de Melo
2021-02-14 2:30 ` Nathan Chancellor
2021-02-17 12:44 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 9+ messages in thread
From: Sedat Dilek @ 2021-02-13 19:11 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Andrii Nakryiko, dwarves, netdev, bpf,
Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
John Fastabend, KP Singh, Nathan Chancellor
On Sat, Feb 13, 2021 at 5:46 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently when processing DWARF function, we check its entrypoint
> against ftrace addresses, assuming that the ftrace address matches
> with function's entrypoint.
>
> This is not the case on some architectures as reported by Nathan
> when building kernel on arm [1].
>
> Fixing the check to take into account the whole function not
> just the entrypoint.
>
> Most of the is_ftrace_func code was contributed by Andrii.
>
> [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested this v2 together with "btf_encoder: sanitize non-regular int
base type" v2 on top of pahole v1.20
Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # Linux v5.11-rc7+ and
LLVM/Clang v12.0.0-rc1 on x86 (64bit)
- Sedat -
> ---
> v2 changes:
> - update functions addr directly [Andrii]
>
> btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index b124ec20a689..80e896961d4e 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
> struct elf_function {
> const char *name;
> unsigned long addr;
> + unsigned long size;
> unsigned long sh_addr;
> bool generated;
> };
> @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>
> functions[functions_cnt].name = name;
> functions[functions_cnt].addr = elf_sym__value(sym);
> + functions[functions_cnt].size = elf_sym__size(sym);
> functions[functions_cnt].sh_addr = sh.sh_addr;
> functions[functions_cnt].generated = false;
> functions_cnt++;
> @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> return 0;
> }
>
> +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> +{
> + __u64 start = func->addr;
> + __u64 addr, end = func->addr + func->size;
> +
> + /*
> + * The invariant here is addr[r] that is the smallest address
> + * that is >= than function start addr. Except the corner case
> + * where there is no such r, but for that we have a final check
> + * in the return.
> + */
> + size_t l = 0, r = count - 1, m;
> +
> + /* make sure we don't use invalid r */
> + if (count == 0)
> + return false;
> +
> + while (l < r) {
> + m = l + (r - l) / 2;
> + addr = addrs[m];
> +
> + if (addr >= start) {
> + /* we satisfy invariant, so tighten r */
> + r = m;
> + } else {
> + /* m is not good enough as l, maybe m + 1 will be */
> + l = m + 1;
> + }
> + }
> +
> + return start <= addrs[r] && addrs[r] < end;
> +}
> +
> static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> {
> __u64 *addrs, count, i;
> @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> * functions[x]::addr is relative address within section
> * and needs to be relocated by adding sh_addr.
> */
> - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> + if (kmod)
> + func->addr += func->sh_addr;
>
> /* Make sure function is within ftrace addresses. */
> - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> + if (is_ftrace_func(func, addrs, count)) {
> /*
> * We iterate over sorted array, so we can easily skip
> * not valid item and move following valid field into
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-13 19:11 ` Sedat Dilek
@ 2021-02-17 12:44 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-17 12:44 UTC (permalink / raw)
To: Sedat Dilek
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Nathan Chancellor
Em Sat, Feb 13, 2021 at 08:11:40PM +0100, Sedat Dilek escreveu:
> On Sat, Feb 13, 2021 at 5:46 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently when processing DWARF function, we check its entrypoint
> > against ftrace addresses, assuming that the ftrace address matches
> > with function's entrypoint.
> >
> > This is not the case on some architectures as reported by Nathan
> > when building kernel on arm [1].
> >
> > Fixing the check to take into account the whole function not
> > just the entrypoint.
> >
> > Most of the is_ftrace_func code was contributed by Andrii.
> >
> > [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Tested this v2 together with "btf_encoder: sanitize non-regular int
> base type" v2 on top of pahole v1.20
>
> Tested-by: Sedat Dilek <sedat.dilek@gmail.com> # Linux v5.11-rc7+ and
> LLVM/Clang v12.0.0-rc1 on x86 (64bit)
Thanks, added to the cset,
- Arnaldo
> - Sedat -
>
> > ---
> > v2 changes:
> > - update functions addr directly [Andrii]
> >
> > btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index b124ec20a689..80e896961d4e 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -36,6 +36,7 @@ struct funcs_layout {
> > struct elf_function {
> > const char *name;
> > unsigned long addr;
> > + unsigned long size;
> > unsigned long sh_addr;
> > bool generated;
> > };
> > @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> >
> > functions[functions_cnt].name = name;
> > functions[functions_cnt].addr = elf_sym__value(sym);
> > + functions[functions_cnt].size = elf_sym__size(sym);
> > functions[functions_cnt].sh_addr = sh.sh_addr;
> > functions[functions_cnt].generated = false;
> > functions_cnt++;
> > @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> > return 0;
> > }
> >
> > +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> > +{
> > + __u64 start = func->addr;
> > + __u64 addr, end = func->addr + func->size;
> > +
> > + /*
> > + * The invariant here is addr[r] that is the smallest address
> > + * that is >= than function start addr. Except the corner case
> > + * where there is no such r, but for that we have a final check
> > + * in the return.
> > + */
> > + size_t l = 0, r = count - 1, m;
> > +
> > + /* make sure we don't use invalid r */
> > + if (count == 0)
> > + return false;
> > +
> > + while (l < r) {
> > + m = l + (r - l) / 2;
> > + addr = addrs[m];
> > +
> > + if (addr >= start) {
> > + /* we satisfy invariant, so tighten r */
> > + r = m;
> > + } else {
> > + /* m is not good enough as l, maybe m + 1 will be */
> > + l = m + 1;
> > + }
> > + }
> > +
> > + return start <= addrs[r] && addrs[r] < end;
> > +}
> > +
> > static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > {
> > __u64 *addrs, count, i;
> > @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > * functions[x]::addr is relative address within section
> > * and needs to be relocated by adding sh_addr.
> > */
> > - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> > + if (kmod)
> > + func->addr += func->sh_addr;
> >
> > /* Make sure function is within ftrace addresses. */
> > - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > + if (is_ftrace_func(func, addrs, count)) {
> > /*
> > * We iterate over sorted array, so we can easily skip
> > * not valid item and move following valid field into
> > --
> > 2.29.2
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-13 16:46 [PATCHv2] btf_encoder: Match ftrace addresses within elf functions Jiri Olsa
2021-02-13 19:11 ` Sedat Dilek
@ 2021-02-14 2:30 ` Nathan Chancellor
2021-02-17 12:45 ` Arnaldo Carvalho de Melo
2021-02-17 12:44 ` Arnaldo Carvalho de Melo
2 siblings, 1 reply; 9+ messages in thread
From: Nathan Chancellor @ 2021-02-14 2:30 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Andrii Nakryiko, dwarves, netdev, bpf,
Yonghong Song, Hao Luo, Martin KaFai Lau, Song Liu,
John Fastabend, KP Singh, Sedat Dilek
On Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa wrote:
> Currently when processing DWARF function, we check its entrypoint
> against ftrace addresses, assuming that the ftrace address matches
> with function's entrypoint.
>
> This is not the case on some architectures as reported by Nathan
> when building kernel on arm [1].
>
> Fixing the check to take into account the whole function not
> just the entrypoint.
>
> Most of the is_ftrace_func code was contributed by Andrii.
>
> [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
I did several builds with CONFIG_DEBUG_INFO_BTF enabled (arm64, ppc64le,
and x86_64) and saw no build errors. I did not do any runtime testing.
Tested-by: Nathan Chancellor <nathan@kernel.org>
> ---
> v2 changes:
> - update functions addr directly [Andrii]
>
> btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index b124ec20a689..80e896961d4e 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
> struct elf_function {
> const char *name;
> unsigned long addr;
> + unsigned long size;
> unsigned long sh_addr;
> bool generated;
> };
> @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>
> functions[functions_cnt].name = name;
> functions[functions_cnt].addr = elf_sym__value(sym);
> + functions[functions_cnt].size = elf_sym__size(sym);
> functions[functions_cnt].sh_addr = sh.sh_addr;
> functions[functions_cnt].generated = false;
> functions_cnt++;
> @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> return 0;
> }
>
> +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> +{
> + __u64 start = func->addr;
> + __u64 addr, end = func->addr + func->size;
> +
> + /*
> + * The invariant here is addr[r] that is the smallest address
> + * that is >= than function start addr. Except the corner case
> + * where there is no such r, but for that we have a final check
> + * in the return.
> + */
> + size_t l = 0, r = count - 1, m;
> +
> + /* make sure we don't use invalid r */
> + if (count == 0)
> + return false;
> +
> + while (l < r) {
> + m = l + (r - l) / 2;
> + addr = addrs[m];
> +
> + if (addr >= start) {
> + /* we satisfy invariant, so tighten r */
> + r = m;
> + } else {
> + /* m is not good enough as l, maybe m + 1 will be */
> + l = m + 1;
> + }
> + }
> +
> + return start <= addrs[r] && addrs[r] < end;
> +}
> +
> static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> {
> __u64 *addrs, count, i;
> @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> * functions[x]::addr is relative address within section
> * and needs to be relocated by adding sh_addr.
> */
> - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> + if (kmod)
> + func->addr += func->sh_addr;
>
> /* Make sure function is within ftrace addresses. */
> - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> + if (is_ftrace_func(func, addrs, count)) {
> /*
> * We iterate over sorted array, so we can easily skip
> * not valid item and move following valid field into
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-14 2:30 ` Nathan Chancellor
@ 2021-02-17 12:45 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-17 12:45 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Sedat Dilek
Em Sat, Feb 13, 2021 at 07:30:48PM -0700, Nathan Chancellor escreveu:
> On Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa wrote:
> > Currently when processing DWARF function, we check its entrypoint
> > against ftrace addresses, assuming that the ftrace address matches
> > with function's entrypoint.
> >
> > This is not the case on some architectures as reported by Nathan
> > when building kernel on arm [1].
> >
> > Fixing the check to take into account the whole function not
> > just the entrypoint.
> >
> > Most of the is_ftrace_func code was contributed by Andrii.
> >
> > [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> I did several builds with CONFIG_DEBUG_INFO_BTF enabled (arm64, ppc64le,
> and x86_64) and saw no build errors. I did not do any runtime testing.
>
> Tested-by: Nathan Chancellor <nathan@kernel.org>
Thanks, added to the cset,
- Arnaldo
> > ---
> > v2 changes:
> > - update functions addr directly [Andrii]
> >
> > btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index b124ec20a689..80e896961d4e 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -36,6 +36,7 @@ struct funcs_layout {
> > struct elf_function {
> > const char *name;
> > unsigned long addr;
> > + unsigned long size;
> > unsigned long sh_addr;
> > bool generated;
> > };
> > @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> >
> > functions[functions_cnt].name = name;
> > functions[functions_cnt].addr = elf_sym__value(sym);
> > + functions[functions_cnt].size = elf_sym__size(sym);
> > functions[functions_cnt].sh_addr = sh.sh_addr;
> > functions[functions_cnt].generated = false;
> > functions_cnt++;
> > @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> > return 0;
> > }
> >
> > +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> > +{
> > + __u64 start = func->addr;
> > + __u64 addr, end = func->addr + func->size;
> > +
> > + /*
> > + * The invariant here is addr[r] that is the smallest address
> > + * that is >= than function start addr. Except the corner case
> > + * where there is no such r, but for that we have a final check
> > + * in the return.
> > + */
> > + size_t l = 0, r = count - 1, m;
> > +
> > + /* make sure we don't use invalid r */
> > + if (count == 0)
> > + return false;
> > +
> > + while (l < r) {
> > + m = l + (r - l) / 2;
> > + addr = addrs[m];
> > +
> > + if (addr >= start) {
> > + /* we satisfy invariant, so tighten r */
> > + r = m;
> > + } else {
> > + /* m is not good enough as l, maybe m + 1 will be */
> > + l = m + 1;
> > + }
> > + }
> > +
> > + return start <= addrs[r] && addrs[r] < end;
> > +}
> > +
> > static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > {
> > __u64 *addrs, count, i;
> > @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > * functions[x]::addr is relative address within section
> > * and needs to be relocated by adding sh_addr.
> > */
> > - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> > + if (kmod)
> > + func->addr += func->sh_addr;
> >
> > /* Make sure function is within ftrace addresses. */
> > - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > + if (is_ftrace_func(func, addrs, count)) {
> > /*
> > * We iterate over sorted array, so we can easily skip
> > * not valid item and move following valid field into
> > --
> > 2.29.2
> >
--
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-13 16:46 [PATCHv2] btf_encoder: Match ftrace addresses within elf functions Jiri Olsa
2021-02-13 19:11 ` Sedat Dilek
2021-02-14 2:30 ` Nathan Chancellor
@ 2021-02-17 12:44 ` Arnaldo Carvalho de Melo
2021-02-17 13:40 ` Sedat Dilek
2 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-17 12:44 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Nathan Chancellor, Sedat Dilek
Em Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa escreveu:
> Currently when processing DWARF function, we check its entrypoint
> against ftrace addresses, assuming that the ftrace address matches
> with function's entrypoint.
>
> This is not the case on some architectures as reported by Nathan
> when building kernel on arm [1].
>
> Fixing the check to take into account the whole function not
> just the entrypoint.
>
> Most of the is_ftrace_func code was contributed by Andrii.
Applied locally, will go out after tests,
- Arnaldo
> [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> v2 changes:
> - update functions addr directly [Andrii]
>
> btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/btf_encoder.c b/btf_encoder.c
> index b124ec20a689..80e896961d4e 100644
> --- a/btf_encoder.c
> +++ b/btf_encoder.c
> @@ -36,6 +36,7 @@ struct funcs_layout {
> struct elf_function {
> const char *name;
> unsigned long addr;
> + unsigned long size;
> unsigned long sh_addr;
> bool generated;
> };
> @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
>
> functions[functions_cnt].name = name;
> functions[functions_cnt].addr = elf_sym__value(sym);
> + functions[functions_cnt].size = elf_sym__size(sym);
> functions[functions_cnt].sh_addr = sh.sh_addr;
> functions[functions_cnt].generated = false;
> functions_cnt++;
> @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> return 0;
> }
>
> +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> +{
> + __u64 start = func->addr;
> + __u64 addr, end = func->addr + func->size;
> +
> + /*
> + * The invariant here is addr[r] that is the smallest address
> + * that is >= than function start addr. Except the corner case
> + * where there is no such r, but for that we have a final check
> + * in the return.
> + */
> + size_t l = 0, r = count - 1, m;
> +
> + /* make sure we don't use invalid r */
> + if (count == 0)
> + return false;
> +
> + while (l < r) {
> + m = l + (r - l) / 2;
> + addr = addrs[m];
> +
> + if (addr >= start) {
> + /* we satisfy invariant, so tighten r */
> + r = m;
> + } else {
> + /* m is not good enough as l, maybe m + 1 will be */
> + l = m + 1;
> + }
> + }
> +
> + return start <= addrs[r] && addrs[r] < end;
> +}
> +
> static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> {
> __u64 *addrs, count, i;
> @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> * functions[x]::addr is relative address within section
> * and needs to be relocated by adding sh_addr.
> */
> - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> + if (kmod)
> + func->addr += func->sh_addr;
>
> /* Make sure function is within ftrace addresses. */
> - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> + if (is_ftrace_func(func, addrs, count)) {
> /*
> * We iterate over sorted array, so we can easily skip
> * not valid item and move following valid field into
> --
> 2.29.2
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-17 12:44 ` Arnaldo Carvalho de Melo
@ 2021-02-17 13:40 ` Sedat Dilek
2021-02-17 13:54 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 9+ messages in thread
From: Sedat Dilek @ 2021-02-17 13:40 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Nathan Chancellor
On Wed, Feb 17, 2021 at 1:44 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa escreveu:
> > Currently when processing DWARF function, we check its entrypoint
> > against ftrace addresses, assuming that the ftrace address matches
> > with function's entrypoint.
> >
> > This is not the case on some architectures as reported by Nathan
> > when building kernel on arm [1].
> >
> > Fixing the check to take into account the whole function not
> > just the entrypoint.
> >
> > Most of the is_ftrace_func code was contributed by Andrii.
>
> Applied locally, will go out after tests,
>
Hi Arnaldo,
Is it possible to have a pahole version 1.21 with this patch and the
one from Yonghong Son?
From my local pahole Git:
$ git log --oneline --no-merges v1.20..
2f83aefdbddf (for-1.20/btf_encoder-ftrace_elf-clang-jolsa-v2)
btf_encoder: Match ftrace addresses within elf functions
f21eafdfc877 (for-1.20/btf_encoder-sanitized_int-clang-yhs-v2)
btf_encoder: sanitize non-regular int base type
Both patches fixes all issues seen so far with LLVM/Clang >=
12.0.0-rc1 and DWARF-v5 and BTF (debug-info) and pahole on
Linux/x86_64 and according to Nathan on Linux/arm64.
Yesterday, I tried with LLVM/Clang 13-git from <apt.llvm.org>.
BTW, Nick's DWARF-v5 patches are pending in <kbuild.git#kbuild> (see [1]).
Personally, I can wait until [1] is in Linus Git.
Please, let me/us know what you are planning.
( I know it is Linux v5.12 merge-window. )
Thanks.
Regards,
- Sedat -
[1] https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/log/?h=kbuild
> - Arnaldo
>
> > [1] https://lore.kernel.org/bpf/20210209034416.GA1669105@ubuntu-m3-large-x86/
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > v2 changes:
> > - update functions addr directly [Andrii]
> >
> > btf_encoder.c | 40 ++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/btf_encoder.c b/btf_encoder.c
> > index b124ec20a689..80e896961d4e 100644
> > --- a/btf_encoder.c
> > +++ b/btf_encoder.c
> > @@ -36,6 +36,7 @@ struct funcs_layout {
> > struct elf_function {
> > const char *name;
> > unsigned long addr;
> > + unsigned long size;
> > unsigned long sh_addr;
> > bool generated;
> > };
> > @@ -98,6 +99,7 @@ static int collect_function(struct btf_elf *btfe, GElf_Sym *sym,
> >
> > functions[functions_cnt].name = name;
> > functions[functions_cnt].addr = elf_sym__value(sym);
> > + functions[functions_cnt].size = elf_sym__size(sym);
> > functions[functions_cnt].sh_addr = sh.sh_addr;
> > functions[functions_cnt].generated = false;
> > functions_cnt++;
> > @@ -236,6 +238,39 @@ get_kmod_addrs(struct btf_elf *btfe, __u64 **paddrs, __u64 *pcount)
> > return 0;
> > }
> >
> > +static int is_ftrace_func(struct elf_function *func, __u64 *addrs, __u64 count)
> > +{
> > + __u64 start = func->addr;
> > + __u64 addr, end = func->addr + func->size;
> > +
> > + /*
> > + * The invariant here is addr[r] that is the smallest address
> > + * that is >= than function start addr. Except the corner case
> > + * where there is no such r, but for that we have a final check
> > + * in the return.
> > + */
> > + size_t l = 0, r = count - 1, m;
> > +
> > + /* make sure we don't use invalid r */
> > + if (count == 0)
> > + return false;
> > +
> > + while (l < r) {
> > + m = l + (r - l) / 2;
> > + addr = addrs[m];
> > +
> > + if (addr >= start) {
> > + /* we satisfy invariant, so tighten r */
> > + r = m;
> > + } else {
> > + /* m is not good enough as l, maybe m + 1 will be */
> > + l = m + 1;
> > + }
> > + }
> > +
> > + return start <= addrs[r] && addrs[r] < end;
> > +}
> > +
> > static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > {
> > __u64 *addrs, count, i;
> > @@ -283,10 +318,11 @@ static int setup_functions(struct btf_elf *btfe, struct funcs_layout *fl)
> > * functions[x]::addr is relative address within section
> > * and needs to be relocated by adding sh_addr.
> > */
> > - __u64 addr = kmod ? func->addr + func->sh_addr : func->addr;
> > + if (kmod)
> > + func->addr += func->sh_addr;
> >
> > /* Make sure function is within ftrace addresses. */
> > - if (bsearch(&addr, addrs, count, sizeof(addrs[0]), addrs_cmp)) {
> > + if (is_ftrace_func(func, addrs, count)) {
> > /*
> > * We iterate over sorted array, so we can easily skip
> > * not valid item and move following valid field into
> > --
> > 2.29.2
> >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-17 13:40 ` Sedat Dilek
@ 2021-02-17 13:54 ` Arnaldo Carvalho de Melo
2021-02-17 18:02 ` Sedat Dilek
0 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-02-17 13:54 UTC (permalink / raw)
To: sedat.dilek, Sedat Dilek
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Nathan Chancellor
On February 17, 2021 10:40:43 AM GMT-03:00, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>On Wed, Feb 17, 2021 at 1:44 PM Arnaldo Carvalho de Melo
><arnaldo.melo@gmail.com> wrote:
>>
>> Em Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa escreveu:
>> > Currently when processing DWARF function, we check its entrypoint
>> > against ftrace addresses, assuming that the ftrace address matches
>> > with function's entrypoint.
>> >
>> > This is not the case on some architectures as reported by Nathan
>> > when building kernel on arm [1].
>> >
>> > Fixing the check to take into account the whole function not
>> > just the entrypoint.
>> >
>> > Most of the is_ftrace_func code was contributed by Andrii.
>>
>> Applied locally, will go out after tests,
>>
>
>Hi Arnaldo,
>
>Is it possible to have a pahole version 1.21 with this patch and the
>one from Yonghong Son?
>
>From my local pahole Git:
>
>$ git log --oneline --no-merges v1.20..
>2f83aefdbddf (for-1.20/btf_encoder-ftrace_elf-clang-jolsa-v2)
>btf_encoder: Match ftrace addresses within elf functions
>f21eafdfc877 (for-1.20/btf_encoder-sanitized_int-clang-yhs-v2)
>btf_encoder: sanitize non-regular int base type
>
>Both patches fixes all issues seen so far with LLVM/Clang >=
>12.0.0-rc1 and DWARF-v5 and BTF (debug-info) and pahole on
>Linux/x86_64 and according to Nathan on Linux/arm64.
>Yesterday, I tried with LLVM/Clang 13-git from <apt.llvm.org>.
>
>BTW, Nick's DWARF-v5 patches are pending in <kbuild.git#kbuild> (see
>[1]).
>
>Personally, I can wait until [1] is in Linus Git.
>
>Please, let me/us know what you are planning.
>( I know it is Linux v5.12 merge-window. )
Sure, next week.
- Arnaldo
>
>Regards,
>- Sedat -
>
>[1]
>https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/log/?h=kbuild
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2] btf_encoder: Match ftrace addresses within elf functions
2021-02-17 13:54 ` Arnaldo Carvalho de Melo
@ 2021-02-17 18:02 ` Sedat Dilek
0 siblings, 0 replies; 9+ messages in thread
From: Sedat Dilek @ 2021-02-17 18:02 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Andrii Nakryiko, dwarves, netdev, bpf, Yonghong Song, Hao Luo,
Martin KaFai Lau, Song Liu, John Fastabend, KP Singh,
Nathan Chancellor
On Wed, Feb 17, 2021 at 2:56 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On February 17, 2021 10:40:43 AM GMT-03:00, Sedat Dilek <sedat.dilek@gmail.com> wrote:
> >On Wed, Feb 17, 2021 at 1:44 PM Arnaldo Carvalho de Melo
> ><arnaldo.melo@gmail.com> wrote:
> >>
> >> Em Sat, Feb 13, 2021 at 05:46:48PM +0100, Jiri Olsa escreveu:
> >> > Currently when processing DWARF function, we check its entrypoint
> >> > against ftrace addresses, assuming that the ftrace address matches
> >> > with function's entrypoint.
> >> >
> >> > This is not the case on some architectures as reported by Nathan
> >> > when building kernel on arm [1].
> >> >
> >> > Fixing the check to take into account the whole function not
> >> > just the entrypoint.
> >> >
> >> > Most of the is_ftrace_func code was contributed by Andrii.
> >>
> >> Applied locally, will go out after tests,
> >>
> >
> >Hi Arnaldo,
> >
> >Is it possible to have a pahole version 1.21 with this patch and the
> >one from Yonghong Son?
> >
> >From my local pahole Git:
> >
> >$ git log --oneline --no-merges v1.20..
> >2f83aefdbddf (for-1.20/btf_encoder-ftrace_elf-clang-jolsa-v2)
> >btf_encoder: Match ftrace addresses within elf functions
> >f21eafdfc877 (for-1.20/btf_encoder-sanitized_int-clang-yhs-v2)
> >btf_encoder: sanitize non-regular int base type
> >
> >Both patches fixes all issues seen so far with LLVM/Clang >=
> >12.0.0-rc1 and DWARF-v5 and BTF (debug-info) and pahole on
> >Linux/x86_64 and according to Nathan on Linux/arm64.
> >Yesterday, I tried with LLVM/Clang 13-git from <apt.llvm.org>.
> >
> >BTW, Nick's DWARF-v5 patches are pending in <kbuild.git#kbuild> (see
> >[1]).
> >
> >Personally, I can wait until [1] is in Linus Git.
> >
> >Please, let me/us know what you are planning.
> >( I know it is Linux v5.12 merge-window. )
>
> Sure, next week.
>
That's OK with me.
- Sedat -
> - Arnaldo
>
> >
> >Regards,
> >- Sedat -
> >
> >[1]
> >https://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git/log/?h=kbuild
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 9+ messages in thread