[PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

With latest bpf-next built with clang lto (thin or full), I hit one test
failures:
  $ ./test_progs -t tcp
  ...
  libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] incompatible with kernel [115303]
  libbpf: failed to load object 'bpf_cubic'
  libbpf: failed to load BPF skeleton 'bpf_cubic': -22
  test_cubic:FAIL:bpf_cubic__open_and_load failed
  #9/2 cubic:FAIL
  ...

The reason of the failure is due to bpf program 'tcp_slow_start'
func signature is different from vmlinux BTF. bpf program uses
the following signature:
  extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
which is identical to the kernel definition in linux:include/net/tcp.h:
  u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
While vmlinux BTF definition like:
  [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
          'tp' type_id=39373
          'acked' type_id=18
  [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
The above is dumped with `bpftool btf dump file vmlinux`.
You can see the ret_type_id is 0 and this caused the problem.

Looking at dwarf, we have:

0x11f2ec67:   DW_TAG_subprogram
                DW_AT_low_pc    (0xffffffff81ed2330)
                DW_AT_high_pc   (0xffffffff81ed235c)
                DW_AT_frame_base        ()
                DW_AT_GNU_all_call_sites        (true)
                DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
...
0x11f2ed66:   DW_TAG_subprogram
                DW_AT_name      ("tcp_slow_start")
                DW_AT_decl_file ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
                DW_AT_decl_line (392)
                DW_AT_prototyped        (true)
                DW_AT_type      (0x11f130c2 "u32")
                DW_AT_external  (true)
                DW_AT_inline    (DW_INL_inlined)

We have a subprogram which has an abstract_origin pointing to
the subprogram prototype with return type. Current one pass
recoding cannot easily resolve this easily since
at the time recoding for 0x11f2ec67, the return type in
0x11f2ed66 has not been resolved.

To simplify implementation, I just added another pass to
go through all functions after recoding pass. This should
resolve the above issue.

With this patch, among total 250999 functions in vmlinux,
4821 functions needs return type adjustment from type id 0
to correct values. The above failed bpf selftest passed too.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
LTO. With this, all self tests are passed except ones due
to global function inlining, static variable promotion etc, which
are not related to pahole.

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 026d137..367ac06 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -2198,6 +2198,42 @@ out:
 	return 0;
 }
 
+static int cu__resolve_func_ret_types(struct cu *cu)
+{
+	struct ptr_table *pt = &cu->functions_table;
+	uint32_t i;
+
+	for (i = 0; i < pt->nr_entries; ++i) {
+		struct tag *tag = pt->entries[i];
+
+		if (tag == NULL || tag->type != 0)
+			continue;
+
+		struct function *fn = tag__function(tag);
+		if (!fn->abstract_origin)
+			continue;
+
+		struct dwarf_tag *dtag = tag->priv;
+		struct dwarf_tag *dfunc;
+		dfunc = dwarf_cu__find_tag_by_ref(cu->priv, &dtag->abstract_origin);
+		if (dfunc == NULL) {
+			tag__print_abstract_origin_not_found(tag);
+			return -1;
+		}
+
+		/*
+		 * Based on what I see it should be a subprogram,
+		 * but double check anyway to ensure I won't mess up
+		 * now and in the future.
+		 */
+		if (dfunc->tag->tag != DW_TAG_subprogram)
+			continue;
+
+		tag->type = dfunc->tag->type;
+	}
+	return 0;
+}
+
 static int cu__recode_dwarf_types_table(struct cu *cu,
 					struct ptr_table *pt,
 					uint32_t i)
@@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
 	/* process merged cu */
 	if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
 		return DWARF_CB_ABORT;
+
+	/*
+	 * for lto build, the function return type may not be
+	 * resolved due to the return type of a subprogram is
+	 * encoded in another subprogram through abstract_origin
+	 * tag. Let us visit all subprograms again to resolve this.
+	 */
+	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
+		return DWARF_CB_ABORT;
+
 	if (finalize_cu_immediately(cus, cu, dcu, conf)
 	    == LSK__STOP_LOADING)
 		return DWARF_CB_ABORT;
-- 
2.30.2

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/1/21 2:36 PM, Yonghong Song wrote:
> With latest bpf-next built with clang lto (thin or full), I hit one test
> failures:
>    $ ./test_progs -t tcp
>    ...
>    libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] incompatible with kernel [115303]
>    libbpf: failed to load object 'bpf_cubic'
>    libbpf: failed to load BPF skeleton 'bpf_cubic': -22
>    test_cubic:FAIL:bpf_cubic__open_and_load failed
>    #9/2 cubic:FAIL
>    ...
> 
> The reason of the failure is due to bpf program 'tcp_slow_start'
> func signature is different from vmlinux BTF. bpf program uses
> the following signature:
>    extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
> which is identical to the kernel definition in linux:include/net/tcp.h:
>    u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
> While vmlinux BTF definition like:
>    [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
>            'tp' type_id=39373
>            'acked' type_id=18
>    [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
> The above is dumped with `bpftool btf dump file vmlinux`.
> You can see the ret_type_id is 0 and this caused the problem.
> 
> Looking at dwarf, we have:
> 
> 0x11f2ec67:   DW_TAG_subprogram
>                  DW_AT_low_pc    (0xffffffff81ed2330)
>                  DW_AT_high_pc   (0xffffffff81ed235c)
>                  DW_AT_frame_base        ()
>                  DW_AT_GNU_all_call_sites        (true)
>                  DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
> ...
> 0x11f2ed66:   DW_TAG_subprogram
>                  DW_AT_name      ("tcp_slow_start")
>                  DW_AT_decl_file ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
>                  DW_AT_decl_line (392)
>                  DW_AT_prototyped        (true)
>                  DW_AT_type      (0x11f130c2 "u32")
>                  DW_AT_external  (true)
>                  DW_AT_inline    (DW_INL_inlined)

David,

Could you help confirm whether DW_AT_abstract_origin at a
DW_TAG_subprogram always points to another DW_TAG_subprogram,
or there are possible other cases?

Thanks,

> 
> We have a subprogram which has an abstract_origin pointing to
> the subprogram prototype with return type. Current one pass
> recoding cannot easily resolve this easily since
> at the time recoding for 0x11f2ec67, the return type in
> 0x11f2ed66 has not been resolved.
> 
> To simplify implementation, I just added another pass to
> go through all functions after recoding pass. This should
> resolve the above issue.
> 
> With this patch, among total 250999 functions in vmlinux,
> 4821 functions needs return type adjustment from type id 0
> to correct values. The above failed bpf selftest passed too.
> 
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>   dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
> 
> Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
> LTO. With this, all self tests are passed except ones due
> to global function inlining, static variable promotion etc, which
> are not related to pahole.
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 026d137..367ac06 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -2198,6 +2198,42 @@ out:
>   	return 0;
>   }
>   
> +static int cu__resolve_func_ret_types(struct cu *cu)
> +{
> +	struct ptr_table *pt = &cu->functions_table;
> +	uint32_t i;
> +
> +	for (i = 0; i < pt->nr_entries; ++i) {
> +		struct tag *tag = pt->entries[i];
> +
> +		if (tag == NULL || tag->type != 0)
> +			continue;
> +
> +		struct function *fn = tag__function(tag);
> +		if (!fn->abstract_origin)
> +			continue;
> +
> +		struct dwarf_tag *dtag = tag->priv;
> +		struct dwarf_tag *dfunc;
> +		dfunc = dwarf_cu__find_tag_by_ref(cu->priv, &dtag->abstract_origin);
> +		if (dfunc == NULL) {
> +			tag__print_abstract_origin_not_found(tag);
> +			return -1;
> +		}
> +
> +		/*
> +		 * Based on what I see it should be a subprogram,
> +		 * but double check anyway to ensure I won't mess up
> +		 * now and in the future.
> +		 */
> +		if (dfunc->tag->tag != DW_TAG_subprogram)
> +			continue;
> +
> +		tag->type = dfunc->tag->type;
> +	}
> +	return 0;
> +}
> +
>   static int cu__recode_dwarf_types_table(struct cu *cu,
>   					struct ptr_table *pt,
>   					uint32_t i)
> @@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>   	/* process merged cu */
>   	if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
>   		return DWARF_CB_ABORT;
> +
> +	/*
> +	 * for lto build, the function return type may not be
> +	 * resolved due to the return type of a subprogram is
> +	 * encoded in another subprogram through abstract_origin
> +	 * tag. Let us visit all subprograms again to resolve this.
> +	 */
> +	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> +		return DWARF_CB_ABORT;
> +
>   	if (finalize_cu_immediately(cus, cu, dcu, conf)
>   	    == LSK__STOP_LOADING)
>   		return DWARF_CB_ABORT;
> 

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[David Blaikie]

On Thu, Apr 1, 2021 at 2:41 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/1/21 2:36 PM, Yonghong Song wrote:
> > With latest bpf-next built with clang lto (thin or full), I hit one test
> > failures:
> >    $ ./test_progs -t tcp
> >    ...
> >    libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] incompatible with kernel [115303]
> >    libbpf: failed to load object 'bpf_cubic'
> >    libbpf: failed to load BPF skeleton 'bpf_cubic': -22
> >    test_cubic:FAIL:bpf_cubic__open_and_load failed
> >    #9/2 cubic:FAIL
> >    ...
> >
> > The reason of the failure is due to bpf program 'tcp_slow_start'
> > func signature is different from vmlinux BTF. bpf program uses
> > the following signature:
> >    extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
> > which is identical to the kernel definition in linux:include/net/tcp.h:
> >    u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
> > While vmlinux BTF definition like:
> >    [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
> >            'tp' type_id=39373
> >            'acked' type_id=18
> >    [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
> > The above is dumped with `bpftool btf dump file vmlinux`.
> > You can see the ret_type_id is 0 and this caused the problem.
> >
> > Looking at dwarf, we have:
> >
> > 0x11f2ec67:   DW_TAG_subprogram
> >                  DW_AT_low_pc    (0xffffffff81ed2330)
> >                  DW_AT_high_pc   (0xffffffff81ed235c)
> >                  DW_AT_frame_base        ()
> >                  DW_AT_GNU_all_call_sites        (true)
> >                  DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
> > ...
> > 0x11f2ed66:   DW_TAG_subprogram
> >                  DW_AT_name      ("tcp_slow_start")
> >                  DW_AT_decl_file ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
> >                  DW_AT_decl_line (392)
> >                  DW_AT_prototyped        (true)
> >                  DW_AT_type      (0x11f130c2 "u32")
> >                  DW_AT_external  (true)
> >                  DW_AT_inline    (DW_INL_inlined)
>
> David,
>
> Could you help confirm whether DW_AT_abstract_origin at a
> DW_TAG_subprogram always points to another DW_TAG_subprogram,
> or there are possible other cases?

That's correct, so far as I understand the spec, specifically DWARFv5
3.3.8.3 says:

"The root entry for a concrete out-of-line instance of a given inlined
subroutine has the same tag as does its associated (abstract) inlined
subroutine entry (that is, tag DW_TAG_subprogram rather than
DW_TAG_inlined_subroutine)."

Though people may come up with novel uses of DWARF features. What
would happen if this constraint were violated/what's your motivation
for asking (I don't quite understand the connection between test_progs
failure description, and this question)

- David

>
> Thanks,
>
> >
> > We have a subprogram which has an abstract_origin pointing to
> > the subprogram prototype with return type. Current one pass
> > recoding cannot easily resolve this easily since
> > at the time recoding for 0x11f2ec67, the return type in
> > 0x11f2ed66 has not been resolved.
> >
> > To simplify implementation, I just added another pass to
> > go through all functions after recoding pass. This should
> > resolve the above issue.
> >
> > With this patch, among total 250999 functions in vmlinux,
> > 4821 functions needs return type adjustment from type id 0
> > to correct values. The above failed bpf selftest passed too.
> >
> > Signed-off-by: Yonghong Song <yhs@fb.com>
> > ---
> >   dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 46 insertions(+)
> >
> > Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
> > LTO. With this, all self tests are passed except ones due
> > to global function inlining, static variable promotion etc, which
> > are not related to pahole.
> >
> > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > index 026d137..367ac06 100644
> > --- a/dwarf_loader.c
> > +++ b/dwarf_loader.c
> > @@ -2198,6 +2198,42 @@ out:
> >       return 0;
> >   }
> >
> > +static int cu__resolve_func_ret_types(struct cu *cu)
> > +{
> > +     struct ptr_table *pt = &cu->functions_table;
> > +     uint32_t i;
> > +
> > +     for (i = 0; i < pt->nr_entries; ++i) {
> > +             struct tag *tag = pt->entries[i];
> > +
> > +             if (tag == NULL || tag->type != 0)
> > +                     continue;
> > +
> > +             struct function *fn = tag__function(tag);
> > +             if (!fn->abstract_origin)
> > +                     continue;
> > +
> > +             struct dwarf_tag *dtag = tag->priv;
> > +             struct dwarf_tag *dfunc;
> > +             dfunc = dwarf_cu__find_tag_by_ref(cu->priv, &dtag->abstract_origin);
> > +             if (dfunc == NULL) {
> > +                     tag__print_abstract_origin_not_found(tag);
> > +                     return -1;
> > +             }
> > +
> > +             /*
> > +              * Based on what I see it should be a subprogram,
> > +              * but double check anyway to ensure I won't mess up
> > +              * now and in the future.
> > +              */
> > +             if (dfunc->tag->tag != DW_TAG_subprogram)
> > +                     continue;
> > +
> > +             tag->type = dfunc->tag->type;
> > +     }
> > +     return 0;
> > +}
> > +
> >   static int cu__recode_dwarf_types_table(struct cu *cu,
> >                                       struct ptr_table *pt,
> >                                       uint32_t i)
> > @@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
> >       /* process merged cu */
> >       if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
> >               return DWARF_CB_ABORT;
> > +
> > +     /*
> > +      * for lto build, the function return type may not be
> > +      * resolved due to the return type of a subprogram is
> > +      * encoded in another subprogram through abstract_origin
> > +      * tag. Let us visit all subprograms again to resolve this.
> > +      */
> > +     if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> > +             return DWARF_CB_ABORT;
> > +
> >       if (finalize_cu_immediately(cus, cu, dcu, conf)
> >           == LSK__STOP_LOADING)
> >               return DWARF_CB_ABORT;
> >

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/1/21 3:27 PM, David Blaikie wrote:
> On Thu, Apr 1, 2021 at 2:41 PM Yonghong Song <yhs@fb.com 
> <mailto:yhs@fb.com>> wrote:
>  >
>  >
>  >
>  > On 4/1/21 2:36 PM, Yonghong Song wrote:
>  > > With latest bpf-next built with clang lto (thin or full), I hit one 
> test
>  > > failures:
>  > >    $ ./test_progs -t tcp
>  > >    ...
>  > >    libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] 
> incompatible with kernel [115303]
>  > >    libbpf: failed to load object 'bpf_cubic'
>  > >    libbpf: failed to load BPF skeleton 'bpf_cubic': -22
>  > >    test_cubic:FAIL:bpf_cubic__open_and_load failed
>  > >    #9/2 cubic:FAIL
>  > >    ...
>  > >
>  > > The reason of the failure is due to bpf program 'tcp_slow_start'
>  > > func signature is different from vmlinux BTF. bpf program uses
>  > > the following signature:
>  > >    extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
>  > > which is identical to the kernel definition in linux:include/net/tcp.h:
>  > >    u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
>  > > While vmlinux BTF definition like:
>  > >    [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
>  > >            'tp' type_id=39373
>  > >            'acked' type_id=18
>  > >    [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
>  > > The above is dumped with `bpftool btf dump file vmlinux`.
>  > > You can see the ret_type_id is 0 and this caused the problem.
>  > >
>  > > Looking at dwarf, we have:
>  > >
>  > > 0x11f2ec67:   DW_TAG_subprogram
>  > >                  DW_AT_low_pc    (0xffffffff81ed2330)
>  > >                  DW_AT_high_pc   (0xffffffff81ed235c)
>  > >                  DW_AT_frame_base        ()
>  > >                  DW_AT_GNU_all_call_sites        (true)
>  > >                  DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
>  > > ...
>  > > 0x11f2ed66:   DW_TAG_subprogram
>  > >                  DW_AT_name      ("tcp_slow_start")
>  > >                  DW_AT_decl_file 
> ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
>  > >                  DW_AT_decl_line (392)
>  > >                  DW_AT_prototyped        (true)
>  > >                  DW_AT_type      (0x11f130c2 "u32")
>  > >                  DW_AT_external  (true)
>  > >                  DW_AT_inline    (DW_INL_inlined)
>  >
>  > David,
>  >
>  > Could you help confirm whether DW_AT_abstract_origin at a
>  > DW_TAG_subprogram always points to another DW_TAG_subprogram,
>  > or there are possible other cases?
> 
> That's correct, so far as I understand the spec, specifically DWARFv5 
> <http://dwarfstd.org/doc/DWARF5.pdf> 
> 3.3.8.3 says:
> 
> "The root entry for a concrete out-of-line instance of a given inlined 
> subroutine has the same tag as does its associated (abstract) inlined 
> subroutine entry (that is, tag DW_TAG_subprogram rather than 
> DW_TAG_inlined_subroutine)."

Thanks. That means that some of my codes in the patch is
dead code.

> 
> Though people may come up with novel uses of DWARF features. What would 
> happen if this constraint were violated/what's your motivation for 
> asking (I don't quite understand the connection between test_progs 
> failure description, and this question)

I have some codes to check the tag associated with abstract_origin
for a subprogram must be a subprogram. Through experiment, I didn't
see a violation, so I wonder that I can get confirmation from you
and then I may delete that code.

The test_progs failure exposed the bug, that is all.

pahole cannot handle all weird usages of dwarf, so I think pahole
is fine only to support well-formed dwarf.

> 
> - David
> 
>  >
>  > Thanks,
>  >
>  > >
>  > > We have a subprogram which has an abstract_origin pointing to
>  > > the subprogram prototype with return type. Current one pass
>  > > recoding cannot easily resolve this easily since
>  > > at the time recoding for 0x11f2ec67, the return type in
>  > > 0x11f2ed66 has not been resolved.
>  > >
>  > > To simplify implementation, I just added another pass to
>  > > go through all functions after recoding pass. This should
>  > > resolve the above issue.
>  > >
>  > > With this patch, among total 250999 functions in vmlinux,
>  > > 4821 functions needs return type adjustment from type id 0
>  > > to correct values. The above failed bpf selftest passed too.
>  > >
>  > > Signed-off-by: Yonghong Song <yhs@fb.com <mailto:yhs@fb.com>>
>  > > ---
>  > >   dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  > >   1 file changed, 46 insertions(+)
>  > >
>  > > Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
>  > > LTO. With this, all self tests are passed except ones due
>  > > to global function inlining, static variable promotion etc, which
>  > > are not related to pahole.
>  > >
>  > > diff --git a/dwarf_loader.c b/dwarf_loader.c
>  > > index 026d137..367ac06 100644
>  > > --- a/dwarf_loader.c
>  > > +++ b/dwarf_loader.c
>  > > @@ -2198,6 +2198,42 @@ out:
>  > >       return 0;
>  > >   }
>  > >
>  > > +static int cu__resolve_func_ret_types(struct cu *cu)
>  > > +{
>  > > +     struct ptr_table *pt = &cu->functions_table;
>  > > +     uint32_t i;
>  > > +
>  > > +     for (i = 0; i < pt->nr_entries; ++i) {
>  > > +             struct tag *tag = pt->entries[i];
>  > > +
>  > > +             if (tag == NULL || tag->type != 0)
>  > > +                     continue;
>  > > +
>  > > +             struct function *fn = tag__function(tag);
>  > > +             if (!fn->abstract_origin)
>  > > +                     continue;
>  > > +
>  > > +             struct dwarf_tag *dtag = tag->priv;
>  > > +             struct dwarf_tag *dfunc;
>  > > +             dfunc = dwarf_cu__find_tag_by_ref(cu->priv, 
> &dtag->abstract_origin);
>  > > +             if (dfunc == NULL) {
>  > > +                     tag__print_abstract_origin_not_found(tag);
>  > > +                     return -1;
>  > > +             }
>  > > +
>  > > +             /*
>  > > +              * Based on what I see it should be a subprogram,
>  > > +              * but double check anyway to ensure I won't mess up
>  > > +              * now and in the future.
>  > > +              */
>  > > +             if (dfunc->tag->tag != DW_TAG_subprogram)
>  > > +                     continue;
>  > > +
>  > > +             tag->type = dfunc->tag->type;
>  > > +     }
>  > > +     return 0;
>  > > +}
>  > > +
>  > >   static int cu__recode_dwarf_types_table(struct cu *cu,
>  > >                                       struct ptr_table *pt,
>  > >                                       uint32_t i)
>  > > @@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct 
> cus *cus, struct conf_load *conf,
>  > >       /* process merged cu */
>  > >       if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
>  > >               return DWARF_CB_ABORT;
>  > > +
>  > > +     /*
>  > > +      * for lto build, the function return type may not be
>  > > +      * resolved due to the return type of a subprogram is
>  > > +      * encoded in another subprogram through abstract_origin
>  > > +      * tag. Let us visit all subprograms again to resolve this.
>  > > +      */
>  > > +     if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
>  > > +             return DWARF_CB_ABORT;
>  > > +
>  > >       if (finalize_cu_immediately(cus, cu, dcu, conf)
>  > >           == LSK__STOP_LOADING)
>  > >               return DWARF_CB_ABORT;
>  > >

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[David Blaikie]

On Thu, Apr 1, 2021 at 4:41 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 4/1/21 3:27 PM, David Blaikie wrote:
> > On Thu, Apr 1, 2021 at 2:41 PM Yonghong Song <yhs@fb.com
> > <mailto:yhs@fb.com>> wrote:
> >  >
> >  >
> >  >
> >  > On 4/1/21 2:36 PM, Yonghong Song wrote:
> >  > > With latest bpf-next built with clang lto (thin or full), I hit one
> > test
> >  > > failures:
> >  > >    $ ./test_progs -t tcp
> >  > >    ...
> >  > >    libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23]
> > incompatible with kernel [115303]
> >  > >    libbpf: failed to load object 'bpf_cubic'
> >  > >    libbpf: failed to load BPF skeleton 'bpf_cubic': -22
> >  > >    test_cubic:FAIL:bpf_cubic__open_and_load failed
> >  > >    #9/2 cubic:FAIL
> >  > >    ...
> >  > >
> >  > > The reason of the failure is due to bpf program 'tcp_slow_start'
> >  > > func signature is different from vmlinux BTF. bpf program uses
> >  > > the following signature:
> >  > >    extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
> >  > > which is identical to the kernel definition in linux:include/net/tcp.h:
> >  > >    u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
> >  > > While vmlinux BTF definition like:
> >  > >    [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
> >  > >            'tp' type_id=39373
> >  > >            'acked' type_id=18
> >  > >    [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
> >  > > The above is dumped with `bpftool btf dump file vmlinux`.
> >  > > You can see the ret_type_id is 0 and this caused the problem.
> >  > >
> >  > > Looking at dwarf, we have:
> >  > >
> >  > > 0x11f2ec67:   DW_TAG_subprogram
> >  > >                  DW_AT_low_pc    (0xffffffff81ed2330)
> >  > >                  DW_AT_high_pc   (0xffffffff81ed235c)
> >  > >                  DW_AT_frame_base        ()
> >  > >                  DW_AT_GNU_all_call_sites        (true)
> >  > >                  DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
> >  > > ...
> >  > > 0x11f2ed66:   DW_TAG_subprogram
> >  > >                  DW_AT_name      ("tcp_slow_start")
> >  > >                  DW_AT_decl_file
> > ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
> >  > >                  DW_AT_decl_line (392)
> >  > >                  DW_AT_prototyped        (true)
> >  > >                  DW_AT_type      (0x11f130c2 "u32")
> >  > >                  DW_AT_external  (true)
> >  > >                  DW_AT_inline    (DW_INL_inlined)
> >  >
> >  > David,
> >  >
> >  > Could you help confirm whether DW_AT_abstract_origin at a
> >  > DW_TAG_subprogram always points to another DW_TAG_subprogram,
> >  > or there are possible other cases?
> >
> > That's correct, so far as I understand the spec, specifically DWARFv5
> > <http://dwarfstd.org/doc/DWARF5.pdf>
> > 3.3.8.3 says:
> >
> > "The root entry for a concrete out-of-line instance of a given inlined
> > subroutine has the same tag as does its associated (abstract) inlined
> > subroutine entry (that is, tag DW_TAG_subprogram rather than
> > DW_TAG_inlined_subroutine)."
>
> Thanks. That means that some of my codes in the patch is
> dead code.
>
> >
> > Though people may come up with novel uses of DWARF features. What would
> > happen if this constraint were violated/what's your motivation for
> > asking (I don't quite understand the connection between test_progs
> > failure description, and this question)
>
> I have some codes to check the tag associated with abstract_origin
> for a subprogram must be a subprogram. Through experiment, I didn't
> see a violation, so I wonder that I can get confirmation from you
> and then I may delete that code.
>
> The test_progs failure exposed the bug, that is all.
>
> pahole cannot handle all weird usages of dwarf, so I think pahole
> is fine only to support well-formed dwarf.

Sounds good. Thanks for the context!

>
> >
> > - David
> >
> >  >
> >  > Thanks,
> >  >
> >  > >
> >  > > We have a subprogram which has an abstract_origin pointing to
> >  > > the subprogram prototype with return type. Current one pass
> >  > > recoding cannot easily resolve this easily since
> >  > > at the time recoding for 0x11f2ec67, the return type in
> >  > > 0x11f2ed66 has not been resolved.
> >  > >
> >  > > To simplify implementation, I just added another pass to
> >  > > go through all functions after recoding pass. This should
> >  > > resolve the above issue.
> >  > >
> >  > > With this patch, among total 250999 functions in vmlinux,
> >  > > 4821 functions needs return type adjustment from type id 0
> >  > > to correct values. The above failed bpf selftest passed too.
> >  > >
> >  > > Signed-off-by: Yonghong Song <yhs@fb.com <mailto:yhs@fb.com>>
> >  > > ---
> >  > >   dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  > >   1 file changed, 46 insertions(+)
> >  > >
> >  > > Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
> >  > > LTO. With this, all self tests are passed except ones due
> >  > > to global function inlining, static variable promotion etc, which
> >  > > are not related to pahole.
> >  > >
> >  > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> >  > > index 026d137..367ac06 100644
> >  > > --- a/dwarf_loader.c
> >  > > +++ b/dwarf_loader.c
> >  > > @@ -2198,6 +2198,42 @@ out:
> >  > >       return 0;
> >  > >   }
> >  > >
> >  > > +static int cu__resolve_func_ret_types(struct cu *cu)
> >  > > +{
> >  > > +     struct ptr_table *pt = &cu->functions_table;
> >  > > +     uint32_t i;
> >  > > +
> >  > > +     for (i = 0; i < pt->nr_entries; ++i) {
> >  > > +             struct tag *tag = pt->entries[i];
> >  > > +
> >  > > +             if (tag == NULL || tag->type != 0)
> >  > > +                     continue;
> >  > > +
> >  > > +             struct function *fn = tag__function(tag);
> >  > > +             if (!fn->abstract_origin)
> >  > > +                     continue;
> >  > > +
> >  > > +             struct dwarf_tag *dtag = tag->priv;
> >  > > +             struct dwarf_tag *dfunc;
> >  > > +             dfunc = dwarf_cu__find_tag_by_ref(cu->priv,
> > &dtag->abstract_origin);
> >  > > +             if (dfunc == NULL) {
> >  > > +                     tag__print_abstract_origin_not_found(tag);
> >  > > +                     return -1;
> >  > > +             }
> >  > > +
> >  > > +             /*
> >  > > +              * Based on what I see it should be a subprogram,
> >  > > +              * but double check anyway to ensure I won't mess up
> >  > > +              * now and in the future.
> >  > > +              */
> >  > > +             if (dfunc->tag->tag != DW_TAG_subprogram)
> >  > > +                     continue;
> >  > > +
> >  > > +             tag->type = dfunc->tag->type;
> >  > > +     }
> >  > > +     return 0;
> >  > > +}
> >  > > +
> >  > >   static int cu__recode_dwarf_types_table(struct cu *cu,
> >  > >                                       struct ptr_table *pt,
> >  > >                                       uint32_t i)
> >  > > @@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct
> > cus *cus, struct conf_load *conf,
> >  > >       /* process merged cu */
> >  > >       if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
> >  > >               return DWARF_CB_ABORT;
> >  > > +
> >  > > +     /*
> >  > > +      * for lto build, the function return type may not be
> >  > > +      * resolved due to the return type of a subprogram is
> >  > > +      * encoded in another subprogram through abstract_origin
> >  > > +      * tag. Let us visit all subprograms again to resolve this.
> >  > > +      */
> >  > > +     if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> >  > > +             return DWARF_CB_ABORT;
> >  > > +
> >  > >       if (finalize_cu_immediately(cus, cu, dcu, conf)
> >  > >           == LSK__STOP_LOADING)
> >  > >               return DWARF_CB_ABORT;
> >  > >

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Thu, Apr 01, 2021 at 05:00:46PM -0700, David Blaikie escreveu:
> On Thu, Apr 1, 2021 at 4:41 PM Yonghong Song <yhs@fb.com> wrote:
> >
> >
> >
> > On 4/1/21 3:27 PM, David Blaikie wrote:
> > > On Thu, Apr 1, 2021 at 2:41 PM Yonghong Song <yhs@fb.com
> > > <mailto:yhs@fb.com>> wrote:
> > >  >
> > >  >
> > >  >
> > >  > On 4/1/21 2:36 PM, Yonghong Song wrote:
> > >  > > With latest bpf-next built with clang lto (thin or full), I hit one
> > > test
> > >  > > failures:
> > >  > >    $ ./test_progs -t tcp
> > >  > >    ...
> > >  > >    libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23]
> > > incompatible with kernel [115303]
> > >  > >    libbpf: failed to load object 'bpf_cubic'
> > >  > >    libbpf: failed to load BPF skeleton 'bpf_cubic': -22
> > >  > >    test_cubic:FAIL:bpf_cubic__open_and_load failed
> > >  > >    #9/2 cubic:FAIL
> > >  > >    ...
> > >  > >
> > >  > > The reason of the failure is due to bpf program 'tcp_slow_start'
> > >  > > func signature is different from vmlinux BTF. bpf program uses
> > >  > > the following signature:
> > >  > >    extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
> > >  > > which is identical to the kernel definition in linux:include/net/tcp.h:
> > >  > >    u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
> > >  > > While vmlinux BTF definition like:
> > >  > >    [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
> > >  > >            'tp' type_id=39373
> > >  > >            'acked' type_id=18
> > >  > >    [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
> > >  > > The above is dumped with `bpftool btf dump file vmlinux`.
> > >  > > You can see the ret_type_id is 0 and this caused the problem.
> > >  > >
> > >  > > Looking at dwarf, we have:
> > >  > >
> > >  > > 0x11f2ec67:   DW_TAG_subprogram
> > >  > >                  DW_AT_low_pc    (0xffffffff81ed2330)
> > >  > >                  DW_AT_high_pc   (0xffffffff81ed235c)
> > >  > >                  DW_AT_frame_base        ()
> > >  > >                  DW_AT_GNU_all_call_sites        (true)
> > >  > >                  DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
> > >  > > ...
> > >  > > 0x11f2ed66:   DW_TAG_subprogram
> > >  > >                  DW_AT_name      ("tcp_slow_start")
> > >  > >                  DW_AT_decl_file
> > > ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
> > >  > >                  DW_AT_decl_line (392)
> > >  > >                  DW_AT_prototyped        (true)
> > >  > >                  DW_AT_type      (0x11f130c2 "u32")
> > >  > >                  DW_AT_external  (true)
> > >  > >                  DW_AT_inline    (DW_INL_inlined)
> > >  >
> > >  > David,
> > >  >
> > >  > Could you help confirm whether DW_AT_abstract_origin at a
> > >  > DW_TAG_subprogram always points to another DW_TAG_subprogram,
> > >  > or there are possible other cases?
> > >
> > > That's correct, so far as I understand the spec, specifically DWARFv5
> > > <http://dwarfstd.org/doc/DWARF5.pdf>
> > > 3.3.8.3 says:
> > >
> > > "The root entry for a concrete out-of-line instance of a given inlined
> > > subroutine has the same tag as does its associated (abstract) inlined
> > > subroutine entry (that is, tag DW_TAG_subprogram rather than
> > > DW_TAG_inlined_subroutine)."
> >
> > Thanks. That means that some of my codes in the patch is
> > dead code.
> >
> > >
> > > Though people may come up with novel uses of DWARF features. What would
> > > happen if this constraint were violated/what's your motivation for
> > > asking (I don't quite understand the connection between test_progs
> > > failure description, and this question)
> >
> > I have some codes to check the tag associated with abstract_origin
> > for a subprogram must be a subprogram. Through experiment, I didn't
> > see a violation, so I wonder that I can get confirmation from you
> > and then I may delete that code.
> >
> > The test_progs failure exposed the bug, that is all.
> >
> > pahole cannot handle all weird usages of dwarf, so I think pahole
> > is fine only to support well-formed dwarf.
> 
> Sounds good. Thanks for the context!

David, since you took the time to go thru the changes and to agree that
Yonghong's fix is good, can I add a:

Acked-by: David Blaikie <dblaikie@gmail.com>

to this patch?

Maybe even a:

Reviewed-by: David Blaikie <dblaikie@gmail.com>

Thanks,

- Arnaldo
 
> >
> > >
> > > - David
> > >
> > >  >
> > >  > Thanks,
> > >  >
> > >  > >
> > >  > > We have a subprogram which has an abstract_origin pointing to
> > >  > > the subprogram prototype with return type. Current one pass
> > >  > > recoding cannot easily resolve this easily since
> > >  > > at the time recoding for 0x11f2ec67, the return type in
> > >  > > 0x11f2ed66 has not been resolved.
> > >  > >
> > >  > > To simplify implementation, I just added another pass to
> > >  > > go through all functions after recoding pass. This should
> > >  > > resolve the above issue.
> > >  > >
> > >  > > With this patch, among total 250999 functions in vmlinux,
> > >  > > 4821 functions needs return type adjustment from type id 0
> > >  > > to correct values. The above failed bpf selftest passed too.
> > >  > >
> > >  > > Signed-off-by: Yonghong Song <yhs@fb.com <mailto:yhs@fb.com>>
> > >  > > ---
> > >  > >   dwarf_loader.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  > >   1 file changed, 46 insertions(+)
> > >  > >
> > >  > > Arnaldo, this is the last known pahole bug in my hand w.r.t. clang
> > >  > > LTO. With this, all self tests are passed except ones due
> > >  > > to global function inlining, static variable promotion etc, which
> > >  > > are not related to pahole.
> > >  > >
> > >  > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > >  > > index 026d137..367ac06 100644
> > >  > > --- a/dwarf_loader.c
> > >  > > +++ b/dwarf_loader.c
> > >  > > @@ -2198,6 +2198,42 @@ out:
> > >  > >       return 0;
> > >  > >   }
> > >  > >
> > >  > > +static int cu__resolve_func_ret_types(struct cu *cu)
> > >  > > +{
> > >  > > +     struct ptr_table *pt = &cu->functions_table;
> > >  > > +     uint32_t i;
> > >  > > +
> > >  > > +     for (i = 0; i < pt->nr_entries; ++i) {
> > >  > > +             struct tag *tag = pt->entries[i];
> > >  > > +
> > >  > > +             if (tag == NULL || tag->type != 0)
> > >  > > +                     continue;
> > >  > > +
> > >  > > +             struct function *fn = tag__function(tag);
> > >  > > +             if (!fn->abstract_origin)
> > >  > > +                     continue;
> > >  > > +
> > >  > > +             struct dwarf_tag *dtag = tag->priv;
> > >  > > +             struct dwarf_tag *dfunc;
> > >  > > +             dfunc = dwarf_cu__find_tag_by_ref(cu->priv,
> > > &dtag->abstract_origin);
> > >  > > +             if (dfunc == NULL) {
> > >  > > +                     tag__print_abstract_origin_not_found(tag);
> > >  > > +                     return -1;
> > >  > > +             }
> > >  > > +
> > >  > > +             /*
> > >  > > +              * Based on what I see it should be a subprogram,
> > >  > > +              * but double check anyway to ensure I won't mess up
> > >  > > +              * now and in the future.
> > >  > > +              */
> > >  > > +             if (dfunc->tag->tag != DW_TAG_subprogram)
> > >  > > +                     continue;
> > >  > > +
> > >  > > +             tag->type = dfunc->tag->type;
> > >  > > +     }
> > >  > > +     return 0;
> > >  > > +}
> > >  > > +
> > >  > >   static int cu__recode_dwarf_types_table(struct cu *cu,
> > >  > >                                       struct ptr_table *pt,
> > >  > >                                       uint32_t i)
> > >  > > @@ -2637,6 +2673,16 @@ static int cus__merge_and_process_cu(struct
> > > cus *cus, struct conf_load *conf,
> > >  > >       /* process merged cu */
> > >  > >       if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
> > >  > >               return DWARF_CB_ABORT;
> > >  > > +
> > >  > > +     /*
> > >  > > +      * for lto build, the function return type may not be
> > >  > > +      * resolved due to the return type of a subprogram is
> > >  > > +      * encoded in another subprogram through abstract_origin
> > >  > > +      * tag. Let us visit all subprograms again to resolve this.
> > >  > > +      */
> > >  > > +     if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> > >  > > +             return DWARF_CB_ABORT;
> > >  > > +
> > >  > >       if (finalize_cu_immediately(cus, cu, dcu, conf)
> > >  > >           == LSK__STOP_LOADING)
> > >  > >               return DWARF_CB_ABORT;
> > >  > >

-- 

- Arnaldo

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Fri, Apr 02, 2021 at 11:04:18AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Apr 01, 2021 at 05:00:46PM -0700, David Blaikie escreveu:
> > On Thu, Apr 1, 2021 at 4:41 PM Yonghong Song <yhs@fb.com> wrote:
> > > On 4/1/21 3:27 PM, David Blaikie wrote:
> > > > Though people may come up with novel uses of DWARF features. What would
> > > > happen if this constraint were violated/what's your motivation for
> > > > asking (I don't quite understand the connection between test_progs
> > > > failure description, and this question)

> > > I have some codes to check the tag associated with abstract_origin
> > > for a subprogram must be a subprogram. Through experiment, I didn't
> > > see a violation, so I wonder that I can get confirmation from you
> > > and then I may delete that code.

> > > The test_progs failure exposed the bug, that is all.

> > > pahole cannot handle all weird usages of dwarf, so I think pahole
> > > is fine only to support well-formed dwarf.

> > Sounds good. Thanks for the context!

> David, since you took the time to go thru the changes and to agree that
> Yonghong's fix is good, can I add a:

> Acked-by: David Blaikie <dblaikie@gmail.com>

> to this patch?

> Maybe even a:

> Reviewed-by: David Blaikie <dblaikie@gmail.com>

What I have is at tmp.master, please take a look and check that
everything is ok, the only think I wished to fix but I think can be left
for later is in the tmp.master branch at:

 git://git.kernel.org/pub/scm/devel/pahole/pahole.git tmp.master

I did some testing for this ret type fix:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master

And for the LTO ELF notes:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=7a79d2d7a573a863aa36fd06f540fe9fa824db4e

The only remaining thing, which I think can be left for 1.22 is:

[acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO
vmlinux.clang.thin.LTO           vmlinux.clang.thin.LTO+ELF_note
[acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO+ELF_note
--- /tmp/btfdiff.dwarf.CtLJpQ	2021-04-02 11:55:09.658433186 -0300
+++ /tmp/btfdiff.btf.d3L3vy	2021-04-02 11:55:09.925439277 -0300
@@ -67255,7 +67255,7 @@ struct cpu_rmap {
 	struct {
 		u16                index;                /*    16     2 */
 		u16                dist;                 /*    18     2 */
-	} near[0]; /*    16     0 */
+	} near[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 5 */
 	/* last cacheline: 16 bytes */
@@ -101181,7 +101181,7 @@ struct linux_efi_memreserve {
 	struct {
 		phys_addr_t        base;                 /*    16     8 */
 		phys_addr_t        size;                 /*    24     8 */
-	} entry[0]; /*    16     0 */
+	} entry[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 4 */
 	/* last cacheline: 16 bytes */
@@ -113516,7 +113516,7 @@ struct netlink_policy_dump_state {
 	struct {
 		const struct nla_policy  * policy;       /*    16     8 */
 		unsigned int       maxtype;              /*    24     4 */
-	} policies[0]; /*    16     0 */
+	} policies[]; /*    16     0 */

 	/* size: 16, cachelines: 1, members: 4 */
 	/* sum members: 12, holes: 1, sum holes: 4 */
[acme@five pahole]$

- Arnaldo

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/2/21 7:57 AM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 02, 2021 at 11:04:18AM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Thu, Apr 01, 2021 at 05:00:46PM -0700, David Blaikie escreveu:
>>> On Thu, Apr 1, 2021 at 4:41 PM Yonghong Song <yhs@fb.com> wrote:
>>>> On 4/1/21 3:27 PM, David Blaikie wrote:
>>>>> Though people may come up with novel uses of DWARF features. What would
>>>>> happen if this constraint were violated/what's your motivation for
>>>>> asking (I don't quite understand the connection between test_progs
>>>>> failure description, and this question)
> 
>>>> I have some codes to check the tag associated with abstract_origin
>>>> for a subprogram must be a subprogram. Through experiment, I didn't
>>>> see a violation, so I wonder that I can get confirmation from you
>>>> and then I may delete that code.
> 
>>>> The test_progs failure exposed the bug, that is all.
> 
>>>> pahole cannot handle all weird usages of dwarf, so I think pahole
>>>> is fine only to support well-formed dwarf.
> 
>>> Sounds good. Thanks for the context!
> 
>> David, since you took the time to go thru the changes and to agree that
>> Yonghong's fix is good, can I add a:
> 
>> Acked-by: David Blaikie <dblaikie@gmail.com>
> 
>> to this patch?
> 
>> Maybe even a:
> 
>> Reviewed-by: David Blaikie <dblaikie@gmail.com>
> 
> What I have is at tmp.master, please take a look and check that
> everything is ok, the only think I wished to fix but I think can be left
> for later is in the tmp.master branch at:
> 
>   git://git.kernel.org/pub/scm/devel/pahole/pahole.git tmp.master

Thanks. I checked out the branch and did some testing with latest clang 
trunk (just pulled in).

With kernel LTO note support, I tested gcc non-lto, and llvm-lto mode, 
it works fine.

Without kernel LTO note support, I tested
    gcc non-lto  <=== ok
    llvm non-lto  <=== not ok
    llvm lto     <=== ok

Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start" issue.
Some previous version of clang does not have this issue.
I double checked the dwarfdump and it is indeed has the same reason
for lto vmlinux. I checked abbrev section and there is no cross-cu
references.

That means we need to adapt this patch
    dwarf_loader: Handle subprogram ret type with abstract_origin properly
for non merging case as well.
The previous patch fixed lto subprogram abstract_origin issue,
I will submit a followup patch for this.

> 
> I did some testing for this ret type fix:
> 
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master
> 
> And for the LTO ELF notes:
> 
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=7a79d2d7a573a863aa36fd06f540fe9fa824db4e
> 
> The only remaining thing, which I think can be left for 1.22 is:
> 
> [acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO
> vmlinux.clang.thin.LTO           vmlinux.clang.thin.LTO+ELF_note
> [acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO+ELF_note
> --- /tmp/btfdiff.dwarf.CtLJpQ	2021-04-02 11:55:09.658433186 -0300
> +++ /tmp/btfdiff.btf.d3L3vy	2021-04-02 11:55:09.925439277 -0300
> @@ -67255,7 +67255,7 @@ struct cpu_rmap {
>   	struct {
>   		u16                index;                /*    16     2 */
>   		u16                dist;                 /*    18     2 */
> -	} near[0]; /*    16     0 */
> +	} near[]; /*    16     0 */
> 
>   	/* size: 16, cachelines: 1, members: 5 */
>   	/* last cacheline: 16 bytes */
> @@ -101181,7 +101181,7 @@ struct linux_efi_memreserve {
>   	struct {
>   		phys_addr_t        base;                 /*    16     8 */
>   		phys_addr_t        size;                 /*    24     8 */
> -	} entry[0]; /*    16     0 */
> +	} entry[]; /*    16     0 */
> 
>   	/* size: 16, cachelines: 1, members: 4 */
>   	/* last cacheline: 16 bytes */
> @@ -113516,7 +113516,7 @@ struct netlink_policy_dump_state {
>   	struct {
>   		const struct nla_policy  * policy;       /*    16     8 */
>   		unsigned int       maxtype;              /*    24     4 */
> -	} policies[0]; /*    16     0 */
> +	} policies[]; /*    16     0 */
> 
>   	/* size: 16, cachelines: 1, members: 4 */
>   	/* sum members: 12, holes: 1, sum holes: 4 */
> [acme@five pahole]$
> 
> - Arnaldo
> 

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/2/21 10:23 AM, Yonghong Song wrote:
> 
> 
> On 4/2/21 7:57 AM, Arnaldo Carvalho de Melo wrote:
>> Em Fri, Apr 02, 2021 at 11:04:18AM -0300, Arnaldo Carvalho de Melo 
>> escreveu:
>>> Em Thu, Apr 01, 2021 at 05:00:46PM -0700, David Blaikie escreveu:
>>>> On Thu, Apr 1, 2021 at 4:41 PM Yonghong Song <yhs@fb.com> wrote:
>>>>> On 4/1/21 3:27 PM, David Blaikie wrote:
>>>>>> Though people may come up with novel uses of DWARF features. What 
>>>>>> would
>>>>>> happen if this constraint were violated/what's your motivation for
>>>>>> asking (I don't quite understand the connection between test_progs
>>>>>> failure description, and this question)
>>
>>>>> I have some codes to check the tag associated with abstract_origin
>>>>> for a subprogram must be a subprogram. Through experiment, I didn't
>>>>> see a violation, so I wonder that I can get confirmation from you
>>>>> and then I may delete that code.
>>
>>>>> The test_progs failure exposed the bug, that is all.
>>
>>>>> pahole cannot handle all weird usages of dwarf, so I think pahole
>>>>> is fine only to support well-formed dwarf.
>>
>>>> Sounds good. Thanks for the context!
>>
>>> David, since you took the time to go thru the changes and to agree that
>>> Yonghong's fix is good, can I add a:
>>
>>> Acked-by: David Blaikie <dblaikie@gmail.com>
>>
>>> to this patch?
>>
>>> Maybe even a:
>>
>>> Reviewed-by: David Blaikie <dblaikie@gmail.com>
>>
>> What I have is at tmp.master, please take a look and check that
>> everything is ok, the only think I wished to fix but I think can be left
>> for later is in the tmp.master branch at:
>>
>>   git://git.kernel.org/pub/scm/devel/pahole/pahole.git tmp.master
> 
> Thanks. I checked out the branch and did some testing with latest clang 
> trunk (just pulled in).
> 
> With kernel LTO note support, I tested gcc non-lto, and llvm-lto mode, 
> it works fine.
> 
> Without kernel LTO note support, I tested
>    gcc non-lto  <=== ok
>    llvm non-lto  <=== not ok
>    llvm lto     <=== ok
> 
> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start" issue.
> Some previous version of clang does not have this issue.
> I double checked the dwarfdump and it is indeed has the same reason
> for lto vmlinux. I checked abbrev section and there is no cross-cu
> references.
> 
> That means we need to adapt this patch
>    dwarf_loader: Handle subprogram ret type with abstract_origin properly
> for non merging case as well.
> The previous patch fixed lto subprogram abstract_origin issue,
> I will submit a followup patch for this.

Actually, the change is pretty simple,

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 5dea837..82d7131 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die 
*die, struct cu *cu)
         int ret = die__process(die, cu);
         if (ret != 0)
                 return ret;
-       return cu__recode_dwarf_types(cu);
+       ret = cu__recode_dwarf_types(cu);
+       if (ret != 0)
+               return ret;
+
+       return cu__resolve_func_ret_types(cu);
  }

Arnaldo, do you just want to fold into previous patches, or
you want me to submit a new one?

> 
>>
>> I did some testing for this ret type fix:
>>
>> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master 
>>
>>
>> And for the LTO ELF notes:
>>
>> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=7a79d2d7a573a863aa36fd06f540fe9fa824db4e 
>>
>>
>> The only remaining thing, which I think can be left for 1.22 is:
>>
>> [acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO
>> vmlinux.clang.thin.LTO           vmlinux.clang.thin.LTO+ELF_note
>> [acme@five pahole]$ btfdiff vmlinux.clang.thin.LTO+ELF_note
>> --- /tmp/btfdiff.dwarf.CtLJpQ    2021-04-02 11:55:09.658433186 -0300
>> +++ /tmp/btfdiff.btf.d3L3vy    2021-04-02 11:55:09.925439277 -0300
>> @@ -67255,7 +67255,7 @@ struct cpu_rmap {
>>       struct {
>>           u16                index;                /*    16     2 */
>>           u16                dist;                 /*    18     2 */
>> -    } near[0]; /*    16     0 */
>> +    } near[]; /*    16     0 */
>>
>>       /* size: 16, cachelines: 1, members: 5 */
>>       /* last cacheline: 16 bytes */
>> @@ -101181,7 +101181,7 @@ struct linux_efi_memreserve {
>>       struct {
>>           phys_addr_t        base;                 /*    16     8 */
>>           phys_addr_t        size;                 /*    24     8 */
>> -    } entry[0]; /*    16     0 */
>> +    } entry[]; /*    16     0 */
>>
>>       /* size: 16, cachelines: 1, members: 4 */
>>       /* last cacheline: 16 bytes */
>> @@ -113516,7 +113516,7 @@ struct netlink_policy_dump_state {
>>       struct {
>>           const struct nla_policy  * policy;       /*    16     8 */
>>           unsigned int       maxtype;              /*    24     4 */
>> -    } policies[0]; /*    16     0 */
>> +    } policies[]; /*    16     0 */
>>
>>       /* size: 16, cachelines: 1, members: 4 */
>>       /* sum members: 12, holes: 1, sum holes: 4 */
>> [acme@five pahole]$
>>
>> - Arnaldo
>>

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo]

On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
>On 4/2/21 10:23 AM, Yonghong Song wrote:
:> Thanks. I checked out the branch and did some testing with latest
>clang 
>> trunk (just pulled in).
>> 
>> With kernel LTO note support, I tested gcc non-lto, and llvm-lto
>mode, 
>> it works fine.
>> 
>> Without kernel LTO note support, I tested
>>    gcc non-lto  <=== ok
>>    llvm non-lto  <=== not ok
>>    llvm lto     <=== ok
>> 
>> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
>issue.
>> Some previous version of clang does not have this issue.
>> I double checked the dwarfdump and it is indeed has the same reason
>> for lto vmlinux. I checked abbrev section and there is no cross-cu
>> references.
>> 
>> That means we need to adapt this patch
>>    dwarf_loader: Handle subprogram ret type with abstract_origin
>properly
>> for non merging case as well.
>> The previous patch fixed lto subprogram abstract_origin issue,
>> I will submit a followup patch for this.
>
>Actually, the change is pretty simple,
>
>diff --git a/dwarf_loader.c b/dwarf_loader.c
>index 5dea837..82d7131 100644
>--- a/dwarf_loader.c
>+++ b/dwarf_loader.c
>@@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die 
>*die, struct cu *cu)
>         int ret = die__process(die, cu);
>         if (ret != 0)
>                 return ret;
>-       return cu__recode_dwarf_types(cu);
>+       ret = cu__recode_dwarf_types(cu);
>+       if (ret != 0)
>+               return ret;
>+
>+       return cu__resolve_func_ret_types(cu);
>  }
>
>Arnaldo, do you just want to fold into previous patches, or
>you want me to submit a new one?

I can take care of that.

And I think it's time for to look at Jiri's test suite... :-)

It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.

Cheers,

- Arnaldo 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Jiri Olsa]

On Fri, Apr 02, 2021 at 03:08:27PM -0300, Arnaldo wrote:
> 
> 
> On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
> >On 4/2/21 10:23 AM, Yonghong Song wrote:
> :> Thanks. I checked out the branch and did some testing with latest
> >clang 
> >> trunk (just pulled in).
> >> 
> >> With kernel LTO note support, I tested gcc non-lto, and llvm-lto
> >mode, 
> >> it works fine.
> >> 
> >> Without kernel LTO note support, I tested
> >>    gcc non-lto  <=== ok
> >>    llvm non-lto  <=== not ok
> >>    llvm lto     <=== ok
> >> 
> >> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
> >issue.
> >> Some previous version of clang does not have this issue.
> >> I double checked the dwarfdump and it is indeed has the same reason
> >> for lto vmlinux. I checked abbrev section and there is no cross-cu
> >> references.
> >> 
> >> That means we need to adapt this patch
> >>    dwarf_loader: Handle subprogram ret type with abstract_origin
> >properly
> >> for non merging case as well.
> >> The previous patch fixed lto subprogram abstract_origin issue,
> >> I will submit a followup patch for this.
> >
> >Actually, the change is pretty simple,
> >
> >diff --git a/dwarf_loader.c b/dwarf_loader.c
> >index 5dea837..82d7131 100644
> >--- a/dwarf_loader.c
> >+++ b/dwarf_loader.c
> >@@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die 
> >*die, struct cu *cu)
> >         int ret = die__process(die, cu);
> >         if (ret != 0)
> >                 return ret;
> >-       return cu__recode_dwarf_types(cu);
> >+       ret = cu__recode_dwarf_types(cu);
> >+       if (ret != 0)
> >+               return ret;
> >+
> >+       return cu__resolve_func_ret_types(cu);
> >  }
> >
> >Arnaldo, do you just want to fold into previous patches, or
> >you want me to submit a new one?
> 
> I can take care of that.
> 
> And I think it's time for to look at Jiri's test suite... :-)
> 
> It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.

heya,
I did not follow this change, but if you put the latest change
into some branch I can run it on top of that

jirka

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/2/21 11:08 AM, Arnaldo wrote:
> 
> 
> On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
>> On 4/2/21 10:23 AM, Yonghong Song wrote:
> :> Thanks. I checked out the branch and did some testing with latest
>> clang
>>> trunk (just pulled in).
>>>
>>> With kernel LTO note support, I tested gcc non-lto, and llvm-lto
>> mode,
>>> it works fine.
>>>
>>> Without kernel LTO note support, I tested
>>>     gcc non-lto  <=== ok
>>>     llvm non-lto  <=== not ok
>>>     llvm lto     <=== ok
>>>
>>> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
>> issue.
>>> Some previous version of clang does not have this issue.
>>> I double checked the dwarfdump and it is indeed has the same reason
>>> for lto vmlinux. I checked abbrev section and there is no cross-cu
>>> references.
>>>
>>> That means we need to adapt this patch
>>>     dwarf_loader: Handle subprogram ret type with abstract_origin
>> properly
>>> for non merging case as well.
>>> The previous patch fixed lto subprogram abstract_origin issue,
>>> I will submit a followup patch for this.
>>
>> Actually, the change is pretty simple,
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index 5dea837..82d7131 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
>> *die, struct cu *cu)
>>          int ret = die__process(die, cu);
>>          if (ret != 0)
>>                  return ret;
>> -       return cu__recode_dwarf_types(cu);
>> +       ret = cu__recode_dwarf_types(cu);
>> +       if (ret != 0)
>> +               return ret;
>> +
>> +       return cu__resolve_func_ret_types(cu);
>>   }
>>
>> Arnaldo, do you just want to fold into previous patches, or
>> you want me to submit a new one?
> 
> I can take care of that.
> 
> And I think it's time for to look at Jiri's test suite... :-)
> 
> It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.

Thanks for taking care of this! Right, 1.21 looks very close.

> 
> Cheers,
> 
> - Arnaldo
> 

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Fri, Apr 02, 2021 at 12:41:47PM -0700, Yonghong Song escreveu:
> 
> 
> On 4/2/21 11:08 AM, Arnaldo wrote:
> > 
> > 
> > On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
> > > On 4/2/21 10:23 AM, Yonghong Song wrote:
> > :> Thanks. I checked out the branch and did some testing with latest
> > > clang
> > > > trunk (just pulled in).
> > > > 
> > > > With kernel LTO note support, I tested gcc non-lto, and llvm-lto
> > > mode,
> > > > it works fine.
> > > > 
> > > > Without kernel LTO note support, I tested
> > > >     gcc non-lto  <=== ok
> > > >     llvm non-lto  <=== not ok
> > > >     llvm lto     <=== ok
> > > > 
> > > > Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
> > > issue.
> > > > Some previous version of clang does not have this issue.
> > > > I double checked the dwarfdump and it is indeed has the same reason
> > > > for lto vmlinux. I checked abbrev section and there is no cross-cu
> > > > references.
> > > > 
> > > > That means we need to adapt this patch
> > > >     dwarf_loader: Handle subprogram ret type with abstract_origin
> > > properly
> > > > for non merging case as well.
> > > > The previous patch fixed lto subprogram abstract_origin issue,
> > > > I will submit a followup patch for this.
> > > 
> > > Actually, the change is pretty simple,
> > > 
> > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > index 5dea837..82d7131 100644
> > > --- a/dwarf_loader.c
> > > +++ b/dwarf_loader.c
> > > @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
> > > *die, struct cu *cu)
> > >          int ret = die__process(die, cu);
> > >          if (ret != 0)
> > >                  return ret;
> > > -       return cu__recode_dwarf_types(cu);
> > > +       ret = cu__recode_dwarf_types(cu);
> > > +       if (ret != 0)
> > > +               return ret;
> > > +
> > > +       return cu__resolve_func_ret_types(cu);
> > >   }
> > > 
> > > Arnaldo, do you just want to fold into previous patches, or
> > > you want me to submit a new one?
> > 
> > I can take care of that.
> > 
> > And I think it's time for to look at Jiri's test suite... :-)
> > 
> > It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.
> 
> Thanks for taking care of this! Right, 1.21 looks very close.

So our HEAD now is this:


commit c03cd5c6ae0cec97d23edcf0a343bbff3df3c96a
Author: Yonghong Song <yhs@fb.com>
Date:   Thu Apr 1 16:55:34 2021 -0700

    dwarf_loader: Handle subprogram ret type with abstract_origin properly
    
    With latest bpf-next built with clang LTO (thin or full), I hit one test
    failures:
    
      $ ./test_progs -t tcp
      ...
      libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] incompatible with kernel [115303]
      libbpf: failed to load object 'bpf_cubic'
      libbpf: failed to load BPF skeleton 'bpf_cubic': -22
      test_cubic:FAIL:bpf_cubic__open_and_load failed
      #9/2 cubic:FAIL
      ...
    
    The reason of the failure is due to bpf program 'tcp_slow_start' func
    signature is different from vmlinux BTF. bpf program uses the following
    signature:
    
      extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
    
    which is identical to the kernel definition in linux:include/net/tcp.h:
    
      u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
    
    While vmlinux BTF definition like:
    
      [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
              'tp' type_id=39373
              'acked' type_id=18
      [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
    
    The above is dumped with `bpftool btf dump file vmlinux`.
    
    You can see the ret_type_id is 0 and this caused the problem.
    
    Looking at dwarf, we have:
    
    0x11f2ec67:   DW_TAG_subprogram
                    DW_AT_low_pc    (0xffffffff81ed2330)
                    DW_AT_high_pc   (0xffffffff81ed235c)
                    DW_AT_frame_base        ()
                    DW_AT_GNU_all_call_sites        (true)
                    DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
    ...
    0x11f2ed66:   DW_TAG_subprogram
                    DW_AT_name      ("tcp_slow_start")
                    DW_AT_decl_file ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
                    DW_AT_decl_line (392)
                    DW_AT_prototyped        (true)
                    DW_AT_type      (0x11f130c2 "u32")
                    DW_AT_external  (true)
                    DW_AT_inline    (DW_INL_inlined)
    
    We have a subprogram which has an abstract_origin pointing to the
    subprogram prototype with return type. Current one pass recoding cannot
    easily resolve this easily since at the time recoding for 0x11f2ec67,
    the return type in 0x11f2ed66 has not been resolved.
    
    To simplify implementation, I just added another pass to go through all
    functions after recoding pass. This should resolve the above issue.
    
    With this patch, among total 250999 functions in vmlinux, 4821 functions
    needs return type adjustment from type id 0 to correct values. The above
    failed bpf selftest passed too.
    
    Committer testing:
    
    Before:
    
      $ pfunct tcp_slow_start
      void tcp_slow_start(struct tcp_sock * tp, u32 acked);
      $
      $ pfunct --prototypes /sys/kernel/btf/vmlinux > before
      $ head before
      int fb_is_primary_device(struct fb_info * info);
      int arch_resume_nosmt(void);
      int relocate_restore_code(void);
      int arch_hibernation_header_restore(void * addr);
      int get_e820_md5(struct e820_table * table, void * buf);
      int arch_hibernation_header_save(void * addr, unsigned int max_size);
      int pfn_is_nosave(long unsigned int pfn);
      int swsusp_arch_resume(void);
      int amd_bus_cpu_online(unsigned int cpu);
      void pci_enable_pci_io_ecs(void);
      $
    
    After:
    
      $ pfunct -F btf ../build/bpf_clang_thin_lto/vmlinux -f tcp_slow_start
      u32 tcp_slow_start(struct tcp_sock * tp, u32 acked);
      $
      $ pfunct -F btf --prototypes ../build/bpf_clang_thin_lto/vmlinux > after
      $
      $ head after
      int fb_is_primary_device(struct fb_info * info);
      int arch_resume_nosmt(void);
      int relocate_restore_code(void);
      int arch_hibernation_header_restore(void * addr);
      int get_e820_md5(struct e820_table * table, void * buf);
      int arch_hibernation_header_save(void * addr, unsigned int max_size);
      int pfn_is_nosave(long unsigned int pfn);
      int swsusp_arch_resume(void);
      int amd_bus_cpu_online(unsigned int cpu);
      void pci_enable_pci_io_ecs(void);
      $
      $ diff -u before after | grep ^+ | wc -l
      1604
      $
    
      $ diff -u before after | grep tcp_slow_start
      -void tcp_slow_start(struct tcp_sock * tp, u32 acked);
      +u32 tcp_slow_start(struct tcp_sock * tp, u32 acked);
      $
      $ diff -u before after | grep ^[+-] | head
      --- before    2021-04-02 11:35:15.578160795 -0300
      +++ after     2021-04-02 11:33:34.204847317 -0300
      -void set_bf_sort(const struct dmi_system_id  * d);
      +int set_bf_sort(const struct dmi_system_id  * d);
      -void raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 val);
      -void raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 * val);
      +int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 val);
      +int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 * val);
      -void xen_find_device_domain_owner(struct pci_dev * dev);
      +int xen_find_device_domain_owner(struct pci_dev * dev);
      $
    
    The same results are obtained if using /sys/kernel/btf/vmlinux after
    rebooting with the kernel built from the ../build/bpf_clang_thin_lto/vmlinux
    file used in the above 'after' examples.
    
    Signed-off-by: Yonghong Song <yhs@fb.com>
    Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
    Acked-by: David Blaikie <dblaikie@gmail.com>
    Cc: Alexei Starovoitov <ast@kernel.org>
    Cc: Bill Wendling <morbo@google.com>
    Cc: Nick Desaulniers <ndesaulniers@google.com>
    Cc: bpf@vger.kernel.org
    Cc: dwarves@vger.kernel.org
    Cc: kernel-team@fb.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/dwarf_loader.c b/dwarf_loader.c
index 026d13789ff912be..5dea8378d7d2a30b 100644
--- a/dwarf_loader.c
+++ b/dwarf_loader.c
@@ -2198,6 +2198,34 @@ out:
 	return 0;
 }
 
+static int cu__resolve_func_ret_types(struct cu *cu)
+{
+	struct ptr_table *pt = &cu->functions_table;
+	uint32_t i;
+
+	for (i = 0; i < pt->nr_entries; ++i) {
+		struct tag *tag = pt->entries[i];
+
+		if (tag == NULL || tag->type != 0)
+			continue;
+
+		struct function *fn = tag__function(tag);
+		if (!fn->abstract_origin)
+			continue;
+
+		struct dwarf_tag *dtag = tag->priv;
+		struct dwarf_tag *dfunc;
+		dfunc = dwarf_cu__find_tag_by_ref(cu->priv, &dtag->abstract_origin);
+		if (dfunc == NULL) {
+			tag__print_abstract_origin_not_found(tag);
+			return -1;
+		}
+
+		tag->type = dfunc->tag->type;
+	}
+	return 0;
+}
+
 static int cu__recode_dwarf_types_table(struct cu *cu,
 					struct ptr_table *pt,
 					uint32_t i)
@@ -2637,6 +2665,16 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
 	/* process merged cu */
 	if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
 		return DWARF_CB_ABORT;
+
+	/*
+	 * for lto build, the function return type may not be
+	 * resolved due to the return type of a subprogram is
+	 * encoded in another subprogram through abstract_origin
+	 * tag. Let us visit all subprograms again to resolve this.
+	 */
+	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
+		return DWARF_CB_ABORT;
+
 	if (finalize_cu_immediately(cus, cu, dcu, conf)
 	    == LSK__STOP_LOADING)
 		return DWARF_CB_ABORT;

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Jiri Olsa]

On Sat, Apr 03, 2021 at 02:08:01PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Apr 02, 2021 at 12:41:47PM -0700, Yonghong Song escreveu:
> > 
> > 
> > On 4/2/21 11:08 AM, Arnaldo wrote:
> > > 
> > > 
> > > On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
> > > > On 4/2/21 10:23 AM, Yonghong Song wrote:
> > > :> Thanks. I checked out the branch and did some testing with latest
> > > > clang
> > > > > trunk (just pulled in).
> > > > > 
> > > > > With kernel LTO note support, I tested gcc non-lto, and llvm-lto
> > > > mode,
> > > > > it works fine.
> > > > > 
> > > > > Without kernel LTO note support, I tested
> > > > >     gcc non-lto  <=== ok
> > > > >     llvm non-lto  <=== not ok
> > > > >     llvm lto     <=== ok
> > > > > 
> > > > > Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
> > > > issue.
> > > > > Some previous version of clang does not have this issue.
> > > > > I double checked the dwarfdump and it is indeed has the same reason
> > > > > for lto vmlinux. I checked abbrev section and there is no cross-cu
> > > > > references.
> > > > > 
> > > > > That means we need to adapt this patch
> > > > >     dwarf_loader: Handle subprogram ret type with abstract_origin
> > > > properly
> > > > > for non merging case as well.
> > > > > The previous patch fixed lto subprogram abstract_origin issue,
> > > > > I will submit a followup patch for this.
> > > > 
> > > > Actually, the change is pretty simple,
> > > > 
> > > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > > index 5dea837..82d7131 100644
> > > > --- a/dwarf_loader.c
> > > > +++ b/dwarf_loader.c
> > > > @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
> > > > *die, struct cu *cu)
> > > >          int ret = die__process(die, cu);
> > > >          if (ret != 0)
> > > >                  return ret;
> > > > -       return cu__recode_dwarf_types(cu);
> > > > +       ret = cu__recode_dwarf_types(cu);
> > > > +       if (ret != 0)
> > > > +               return ret;
> > > > +
> > > > +       return cu__resolve_func_ret_types(cu);
> > > >   }
> > > > 
> > > > Arnaldo, do you just want to fold into previous patches, or
> > > > you want me to submit a new one?
> > > 
> > > I can take care of that.
> > > 
> > > And I think it's time for to look at Jiri's test suite... :-)

heya,
the test did not get change in generated BTF data with the change below,
so looks good

jirka

> > > 
> > > It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.
> > 
> > Thanks for taking care of this! Right, 1.21 looks very close.
> 
> So our HEAD now is this:
> 
> 
> commit c03cd5c6ae0cec97d23edcf0a343bbff3df3c96a
> Author: Yonghong Song <yhs@fb.com>
> Date:   Thu Apr 1 16:55:34 2021 -0700
> 
>     dwarf_loader: Handle subprogram ret type with abstract_origin properly
>     
>     With latest bpf-next built with clang LTO (thin or full), I hit one test
>     failures:
>     
>       $ ./test_progs -t tcp
>       ...
>       libbpf: extern (func ksym) 'tcp_slow_start': func_proto [23] incompatible with kernel [115303]
>       libbpf: failed to load object 'bpf_cubic'
>       libbpf: failed to load BPF skeleton 'bpf_cubic': -22
>       test_cubic:FAIL:bpf_cubic__open_and_load failed
>       #9/2 cubic:FAIL
>       ...
>     
>     The reason of the failure is due to bpf program 'tcp_slow_start' func
>     signature is different from vmlinux BTF. bpf program uses the following
>     signature:
>     
>       extern __u32 tcp_slow_start(struct tcp_sock *tp, __u32 acked);
>     
>     which is identical to the kernel definition in linux:include/net/tcp.h:
>     
>       u32 tcp_slow_start(struct tcp_sock *tp, u32 acked);
>     
>     While vmlinux BTF definition like:
>     
>       [115303] FUNC_PROTO '(anon)' ret_type_id=0 vlen=2
>               'tp' type_id=39373
>               'acked' type_id=18
>       [115304] FUNC 'tcp_slow_start' type_id=115303 linkage=static
>     
>     The above is dumped with `bpftool btf dump file vmlinux`.
>     
>     You can see the ret_type_id is 0 and this caused the problem.
>     
>     Looking at dwarf, we have:
>     
>     0x11f2ec67:   DW_TAG_subprogram
>                     DW_AT_low_pc    (0xffffffff81ed2330)
>                     DW_AT_high_pc   (0xffffffff81ed235c)
>                     DW_AT_frame_base        ()
>                     DW_AT_GNU_all_call_sites        (true)
>                     DW_AT_abstract_origin   (0x11f2ed66 "tcp_slow_start")
>     ...
>     0x11f2ed66:   DW_TAG_subprogram
>                     DW_AT_name      ("tcp_slow_start")
>                     DW_AT_decl_file ("/home/yhs/work/bpf-next/net/ipv4/tcp_cong.c")
>                     DW_AT_decl_line (392)
>                     DW_AT_prototyped        (true)
>                     DW_AT_type      (0x11f130c2 "u32")
>                     DW_AT_external  (true)
>                     DW_AT_inline    (DW_INL_inlined)
>     
>     We have a subprogram which has an abstract_origin pointing to the
>     subprogram prototype with return type. Current one pass recoding cannot
>     easily resolve this easily since at the time recoding for 0x11f2ec67,
>     the return type in 0x11f2ed66 has not been resolved.
>     
>     To simplify implementation, I just added another pass to go through all
>     functions after recoding pass. This should resolve the above issue.
>     
>     With this patch, among total 250999 functions in vmlinux, 4821 functions
>     needs return type adjustment from type id 0 to correct values. The above
>     failed bpf selftest passed too.
>     
>     Committer testing:
>     
>     Before:
>     
>       $ pfunct tcp_slow_start
>       void tcp_slow_start(struct tcp_sock * tp, u32 acked);
>       $
>       $ pfunct --prototypes /sys/kernel/btf/vmlinux > before
>       $ head before
>       int fb_is_primary_device(struct fb_info * info);
>       int arch_resume_nosmt(void);
>       int relocate_restore_code(void);
>       int arch_hibernation_header_restore(void * addr);
>       int get_e820_md5(struct e820_table * table, void * buf);
>       int arch_hibernation_header_save(void * addr, unsigned int max_size);
>       int pfn_is_nosave(long unsigned int pfn);
>       int swsusp_arch_resume(void);
>       int amd_bus_cpu_online(unsigned int cpu);
>       void pci_enable_pci_io_ecs(void);
>       $
>     
>     After:
>     
>       $ pfunct -F btf ../build/bpf_clang_thin_lto/vmlinux -f tcp_slow_start
>       u32 tcp_slow_start(struct tcp_sock * tp, u32 acked);
>       $
>       $ pfunct -F btf --prototypes ../build/bpf_clang_thin_lto/vmlinux > after
>       $
>       $ head after
>       int fb_is_primary_device(struct fb_info * info);
>       int arch_resume_nosmt(void);
>       int relocate_restore_code(void);
>       int arch_hibernation_header_restore(void * addr);
>       int get_e820_md5(struct e820_table * table, void * buf);
>       int arch_hibernation_header_save(void * addr, unsigned int max_size);
>       int pfn_is_nosave(long unsigned int pfn);
>       int swsusp_arch_resume(void);
>       int amd_bus_cpu_online(unsigned int cpu);
>       void pci_enable_pci_io_ecs(void);
>       $
>       $ diff -u before after | grep ^+ | wc -l
>       1604
>       $
>     
>       $ diff -u before after | grep tcp_slow_start
>       -void tcp_slow_start(struct tcp_sock * tp, u32 acked);
>       +u32 tcp_slow_start(struct tcp_sock * tp, u32 acked);
>       $
>       $ diff -u before after | grep ^[+-] | head
>       --- before    2021-04-02 11:35:15.578160795 -0300
>       +++ after     2021-04-02 11:33:34.204847317 -0300
>       -void set_bf_sort(const struct dmi_system_id  * d);
>       +int set_bf_sort(const struct dmi_system_id  * d);
>       -void raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 val);
>       -void raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 * val);
>       +int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 val);
>       +int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 * val);
>       -void xen_find_device_domain_owner(struct pci_dev * dev);
>       +int xen_find_device_domain_owner(struct pci_dev * dev);
>       $
>     
>     The same results are obtained if using /sys/kernel/btf/vmlinux after
>     rebooting with the kernel built from the ../build/bpf_clang_thin_lto/vmlinux
>     file used in the above 'after' examples.
>     
>     Signed-off-by: Yonghong Song <yhs@fb.com>
>     Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>     Acked-by: David Blaikie <dblaikie@gmail.com>
>     Cc: Alexei Starovoitov <ast@kernel.org>
>     Cc: Bill Wendling <morbo@google.com>
>     Cc: Nick Desaulniers <ndesaulniers@google.com>
>     Cc: bpf@vger.kernel.org
>     Cc: dwarves@vger.kernel.org
>     Cc: kernel-team@fb.com
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> diff --git a/dwarf_loader.c b/dwarf_loader.c
> index 026d13789ff912be..5dea8378d7d2a30b 100644
> --- a/dwarf_loader.c
> +++ b/dwarf_loader.c
> @@ -2198,6 +2198,34 @@ out:
>  	return 0;
>  }
>  
> +static int cu__resolve_func_ret_types(struct cu *cu)
> +{
> +	struct ptr_table *pt = &cu->functions_table;
> +	uint32_t i;
> +
> +	for (i = 0; i < pt->nr_entries; ++i) {
> +		struct tag *tag = pt->entries[i];
> +
> +		if (tag == NULL || tag->type != 0)
> +			continue;
> +
> +		struct function *fn = tag__function(tag);
> +		if (!fn->abstract_origin)
> +			continue;
> +
> +		struct dwarf_tag *dtag = tag->priv;
> +		struct dwarf_tag *dfunc;
> +		dfunc = dwarf_cu__find_tag_by_ref(cu->priv, &dtag->abstract_origin);
> +		if (dfunc == NULL) {
> +			tag__print_abstract_origin_not_found(tag);
> +			return -1;
> +		}
> +
> +		tag->type = dfunc->tag->type;
> +	}
> +	return 0;
> +}
> +
>  static int cu__recode_dwarf_types_table(struct cu *cu,
>  					struct ptr_table *pt,
>  					uint32_t i)
> @@ -2637,6 +2665,16 @@ static int cus__merge_and_process_cu(struct cus *cus, struct conf_load *conf,
>  	/* process merged cu */
>  	if (cu__recode_dwarf_types(cu) != LSK__KEEPIT)
>  		return DWARF_CB_ABORT;
> +
> +	/*
> +	 * for lto build, the function return type may not be
> +	 * resolved due to the return type of a subprogram is
> +	 * encoded in another subprogram through abstract_origin
> +	 * tag. Let us visit all subprograms again to resolve this.
> +	 */
> +	if (cu__resolve_func_ret_types(cu) != LSK__KEEPIT)
> +		return DWARF_CB_ABORT;
> +
>  	if (finalize_cu_immediately(cus, cu, dcu, conf)
>  	    == LSK__STOP_LOADING)
>  		return DWARF_CB_ABORT;
> 

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/2/21 11:08 AM, Arnaldo wrote:
> 
> 
> On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
>> On 4/2/21 10:23 AM, Yonghong Song wrote:
> :> Thanks. I checked out the branch and did some testing with latest
>> clang
>>> trunk (just pulled in).
>>>
>>> With kernel LTO note support, I tested gcc non-lto, and llvm-lto
>> mode,
>>> it works fine.
>>>
>>> Without kernel LTO note support, I tested
>>>     gcc non-lto  <=== ok
>>>     llvm non-lto  <=== not ok
>>>     llvm lto     <=== ok
>>>
>>> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
>> issue.
>>> Some previous version of clang does not have this issue.
>>> I double checked the dwarfdump and it is indeed has the same reason
>>> for lto vmlinux. I checked abbrev section and there is no cross-cu
>>> references.
>>>
>>> That means we need to adapt this patch
>>>     dwarf_loader: Handle subprogram ret type with abstract_origin
>> properly
>>> for non merging case as well.
>>> The previous patch fixed lto subprogram abstract_origin issue,
>>> I will submit a followup patch for this.
>>
>> Actually, the change is pretty simple,
>>
>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>> index 5dea837..82d7131 100644
>> --- a/dwarf_loader.c
>> +++ b/dwarf_loader.c
>> @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
>> *die, struct cu *cu)
>>          int ret = die__process(die, cu);
>>          if (ret != 0)
>>                  return ret;
>> -       return cu__recode_dwarf_types(cu);
>> +       ret = cu__recode_dwarf_types(cu);
>> +       if (ret != 0)
>> +               return ret;
>> +
>> +       return cu__resolve_func_ret_types(cu);
>>   }
>>
>> Arnaldo, do you just want to fold into previous patches, or
>> you want me to submit a new one?
> 
> I can take care of that.

Arnaldo, just in case that you missed it, please remember
to fold the above changes to the patch:
    [PATCH dwarves] dwarf_loader: handle subprogram ret type with 
abstract_origin properly
Thanks!

> 
> And I think it's time for to look at Jiri's test suite... :-)
> 
> It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.
> 
> Cheers,
> 
> - Arnaldo
> 

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
> On 4/2/21 11:08 AM, Arnaldo wrote:
> > On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
> > > On 4/2/21 10:23 AM, Yonghong Song wrote:
> > :> Thanks. I checked out the branch and did some testing with latest
> > > clang
> > > > trunk (just pulled in).
> > > > 
> > > > With kernel LTO note support, I tested gcc non-lto, and llvm-lto
> > > mode,
> > > > it works fine.
> > > > 
> > > > Without kernel LTO note support, I tested
> > > >     gcc non-lto  <=== ok
> > > >     llvm non-lto  <=== not ok
> > > >     llvm lto     <=== ok
> > > > 
> > > > Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
> > > issue.
> > > > Some previous version of clang does not have this issue.
> > > > I double checked the dwarfdump and it is indeed has the same reason
> > > > for lto vmlinux. I checked abbrev section and there is no cross-cu
> > > > references.
> > > > 
> > > > That means we need to adapt this patch
> > > >     dwarf_loader: Handle subprogram ret type with abstract_origin
> > > properly
> > > > for non merging case as well.
> > > > The previous patch fixed lto subprogram abstract_origin issue,
> > > > I will submit a followup patch for this.
> > > 
> > > Actually, the change is pretty simple,
> > > 
> > > diff --git a/dwarf_loader.c b/dwarf_loader.c
> > > index 5dea837..82d7131 100644
> > > --- a/dwarf_loader.c
> > > +++ b/dwarf_loader.c
> > > @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
> > > *die, struct cu *cu)
> > >          int ret = die__process(die, cu);
> > >          if (ret != 0)
> > >                  return ret;
> > > -       return cu__recode_dwarf_types(cu);
> > > +       ret = cu__recode_dwarf_types(cu);
> > > +       if (ret != 0)
> > > +               return ret;
> > > +
> > > +       return cu__resolve_func_ret_types(cu);
> > >   }
> > > 
> > > Arnaldo, do you just want to fold into previous patches, or
> > > you want me to submit a new one?
> > 
> > I can take care of that.
> 
> Arnaldo, just in case that you missed it, please remember
> to fold the above changes to the patch:
>    [PATCH dwarves] dwarf_loader: handle subprogram ret type with
> abstract_origin properly
> Thanks!

Its there, I did it Sunday, IIRC:

https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad

@@ -2295,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die *die, struct cu *cu)
 	int ret = die__process(die, cu);
 	if (ret != 0)
 		return ret;
-	return cu__recode_dwarf_types(cu);
+	ret = cu__recode_dwarf_types(cu);
+	if (ret != 0)
+		return ret;
+
+	return cu__resolve_func_ret_types(cu);
 }

 static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,

----

My latest tests were all with it in place.

- Arnaldo
 
> > 
> > And I think it's time for to look at Jiri's test suite... :-)
> > 
> > It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.
> > 
> > Cheers,
> > 
> > - Arnaldo
> > 

-- 

- Arnaldo

[RFT] prepping up pahole 1.21, wanna test it? was Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Wed, Apr 07, 2021 at 12:37:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
> > Arnaldo, just in case that you missed it, please remember
> > to fold the above changes to the patch:
> >    [PATCH dwarves] dwarf_loader: handle subprogram ret type with
> > abstract_origin properly
> > Thanks!
> 
> Its there, I did it Sunday, IIRC:
> 
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad

So I pushed it all to the master branch, hopefully some more people may
feel encouraged to give it a try for the various things it fixes since
1.20:

[acme@quaco pahole]$ git log --oneline v1.20..
ae0b7dde1fd50b12 (HEAD -> master, origin/tmp.master, origin/master, origin/HEAD, github/master, five/master, acme.korg/tmp.master, acme.korg/master) dwarf_loader: Handle DWARF5 DW_OP_addrx properly
9adb014930f31c66 dwarf_loader: Handle subprogram ret type with abstract_origin properly
5752d1951d081a80 dwarf_loader: Check .notes section for LTO build info
209e45424ff4a22d dwarf_loader: Check .debug_abbrev for cross-CU references
39227909db3cc2c2 dwarf_loader: Permit merging all DWARF CU's for clang LTO built binary
763475ca1101ccfe dwarf_loader: Factor out common code to initialize a cu
d0d3fbd4744953e8 dwarf_loader: Permit a flexible HASHTAGS__BITS
ffe0ef4d73906c18 btf: Add --btf_gen_all flag
de708b33114d42c2 btf: Add support for the floating-point types
4b7f8c04d009942b fprintf: Honour conf_fprintf.hex when printing enumerations
f2889ff163726336 Avoid warning when building with NDEBUG
8e1f8c904e303d5d btf_encoder: Match ftrace addresses within ELF functions
9fecc77ed82d429f dwarf_loader: Use a better hashing function, from libbpf
0125de3a4c055cdf btf_encoder: Funnel ELF error reporting through a macro
7d8e829f636f47ab btf_encoder: Sanitize non-regular int base type
[acme@quaco pahole]$

- Arnaldo

Re: [RFT] prepping up pahole 1.21, wanna test it? was Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/7/21 8:40 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 07, 2021 at 12:37:09PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
>>> Arnaldo, just in case that you missed it, please remember
>>> to fold the above changes to the patch:
>>>     [PATCH dwarves] dwarf_loader: handle subprogram ret type with
>>> abstract_origin properly
>>> Thanks!
>>
>> Its there, I did it Sunday, IIRC:
>>
>> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad
> 
> So I pushed it all to the master branch, hopefully some more people may
> feel encouraged to give it a try for the various things it fixes since
> 1.20:
> 
> [acme@quaco pahole]$ git log --oneline v1.20..
> ae0b7dde1fd50b12 (HEAD -> master, origin/tmp.master, origin/master, origin/HEAD, github/master, five/master, acme.korg/tmp.master, acme.korg/master) dwarf_loader: Handle DWARF5 DW_OP_addrx properly
> 9adb014930f31c66 dwarf_loader: Handle subprogram ret type with abstract_origin properly
> 5752d1951d081a80 dwarf_loader: Check .notes section for LTO build info
> 209e45424ff4a22d dwarf_loader: Check .debug_abbrev for cross-CU references
> 39227909db3cc2c2 dwarf_loader: Permit merging all DWARF CU's for clang LTO built binary
> 763475ca1101ccfe dwarf_loader: Factor out common code to initialize a cu
> d0d3fbd4744953e8 dwarf_loader: Permit a flexible HASHTAGS__BITS
> ffe0ef4d73906c18 btf: Add --btf_gen_all flag
> de708b33114d42c2 btf: Add support for the floating-point types
> 4b7f8c04d009942b fprintf: Honour conf_fprintf.hex when printing enumerations
> f2889ff163726336 Avoid warning when building with NDEBUG
> 8e1f8c904e303d5d btf_encoder: Match ftrace addresses within ELF functions
> 9fecc77ed82d429f dwarf_loader: Use a better hashing function, from libbpf
> 0125de3a4c055cdf btf_encoder: Funnel ELF error reporting through a macro
> 7d8e829f636f47ab btf_encoder: Sanitize non-regular int base type
> [acme@quaco pahole]$

I tested with llvm-project "main" branch and latest bpf-next.
clang non-lto, dwarf4/5: okay
clang thin-lto, dwarf4/5: okay
gcc (8.4.1, non-lto, default dwarf4): okay

So pahole looks good. Thanks!

> 
> - Arnaldo
> 

Re: [RFT] prepping up pahole 1.21, wanna test it? was Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Wed, Apr 07, 2021 at 03:53:32PM -0700, Yonghong Song escreveu:
> 
> 
> On 4/7/21 8:40 AM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 07, 2021 at 12:37:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
> > > > Arnaldo, just in case that you missed it, please remember
> > > > to fold the above changes to the patch:
> > > >     [PATCH dwarves] dwarf_loader: handle subprogram ret type with
> > > > abstract_origin properly
> > > > Thanks!
> > > 
> > > Its there, I did it Sunday, IIRC:
> > > 
> > > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad
> > 
> > So I pushed it all to the master branch, hopefully some more people may
> > feel encouraged to give it a try for the various things it fixes since
> > 1.20:
> > 
> > [acme@quaco pahole]$ git log --oneline v1.20..
> > ae0b7dde1fd50b12 (HEAD -> master, origin/tmp.master, origin/master, origin/HEAD, github/master, five/master, acme.korg/tmp.master, acme.korg/master) dwarf_loader: Handle DWARF5 DW_OP_addrx properly
> > 9adb014930f31c66 dwarf_loader: Handle subprogram ret type with abstract_origin properly
> > 5752d1951d081a80 dwarf_loader: Check .notes section for LTO build info
> > 209e45424ff4a22d dwarf_loader: Check .debug_abbrev for cross-CU references
> > 39227909db3cc2c2 dwarf_loader: Permit merging all DWARF CU's for clang LTO built binary
> > 763475ca1101ccfe dwarf_loader: Factor out common code to initialize a cu
> > d0d3fbd4744953e8 dwarf_loader: Permit a flexible HASHTAGS__BITS
> > ffe0ef4d73906c18 btf: Add --btf_gen_all flag
> > de708b33114d42c2 btf: Add support for the floating-point types
> > 4b7f8c04d009942b fprintf: Honour conf_fprintf.hex when printing enumerations
> > f2889ff163726336 Avoid warning when building with NDEBUG
> > 8e1f8c904e303d5d btf_encoder: Match ftrace addresses within ELF functions
> > 9fecc77ed82d429f dwarf_loader: Use a better hashing function, from libbpf
> > 0125de3a4c055cdf btf_encoder: Funnel ELF error reporting through a macro
> > 7d8e829f636f47ab btf_encoder: Sanitize non-regular int base type
> > [acme@quaco pahole]$
> 
> I tested with llvm-project "main" branch and latest bpf-next.
> clang non-lto, dwarf4/5: okay
> clang thin-lto, dwarf4/5: okay
> gcc (8.4.1, non-lto, default dwarf4): okay
> 
> So pahole looks good. Thanks!

Thanks a lot, in time for a friday release! :-)

- Arnaldo

Re: [RFT] prepping up pahole 1.21, wanna test it? was Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Jiri Olsa]

On Wed, Apr 07, 2021 at 12:40:18PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 07, 2021 at 12:37:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
> > > Arnaldo, just in case that you missed it, please remember
> > > to fold the above changes to the patch:
> > >    [PATCH dwarves] dwarf_loader: handle subprogram ret type with
> > > abstract_origin properly
> > > Thanks!
> > 
> > Its there, I did it Sunday, IIRC:
> > 
> > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad
> 
> So I pushed it all to the master branch, hopefully some more people may
> feel encouraged to give it a try for the various things it fixes since
> 1.20:

heya,
the test script passed

jirka

> 
> [acme@quaco pahole]$ git log --oneline v1.20..
> ae0b7dde1fd50b12 (HEAD -> master, origin/tmp.master, origin/master, origin/HEAD, github/master, five/master, acme.korg/tmp.master, acme.korg/master) dwarf_loader: Handle DWARF5 DW_OP_addrx properly
> 9adb014930f31c66 dwarf_loader: Handle subprogram ret type with abstract_origin properly
> 5752d1951d081a80 dwarf_loader: Check .notes section for LTO build info
> 209e45424ff4a22d dwarf_loader: Check .debug_abbrev for cross-CU references
> 39227909db3cc2c2 dwarf_loader: Permit merging all DWARF CU's for clang LTO built binary
> 763475ca1101ccfe dwarf_loader: Factor out common code to initialize a cu
> d0d3fbd4744953e8 dwarf_loader: Permit a flexible HASHTAGS__BITS
> ffe0ef4d73906c18 btf: Add --btf_gen_all flag
> de708b33114d42c2 btf: Add support for the floating-point types
> 4b7f8c04d009942b fprintf: Honour conf_fprintf.hex when printing enumerations
> f2889ff163726336 Avoid warning when building with NDEBUG
> 8e1f8c904e303d5d btf_encoder: Match ftrace addresses within ELF functions
> 9fecc77ed82d429f dwarf_loader: Use a better hashing function, from libbpf
> 0125de3a4c055cdf btf_encoder: Funnel ELF error reporting through a macro
> 7d8e829f636f47ab btf_encoder: Sanitize non-regular int base type
> [acme@quaco pahole]$
> 
> - Arnaldo
> 

Re: [RFT] prepping up pahole 1.21, wanna test it? was Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Arnaldo Carvalho de Melo]

Em Thu, Apr 08, 2021 at 11:23:32PM +0200, Jiri Olsa escreveu:
> On Wed, Apr 07, 2021 at 12:40:18PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Apr 07, 2021 at 12:37:09PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
> > > > Arnaldo, just in case that you missed it, please remember
> > > > to fold the above changes to the patch:
> > > >    [PATCH dwarves] dwarf_loader: handle subprogram ret type with
> > > > abstract_origin properly
> > > > Thanks!
> > > 
> > > Its there, I did it Sunday, IIRC:
> > > 
> > > https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad
> > 
> > So I pushed it all to the master branch, hopefully some more people may
> > feel encouraged to give it a try for the various things it fixes since
> > 1.20:
> 
> heya,
> the test script passed

Thanks a lot for testing!

- Arnaldo
 
> jirka
> 
> > 
> > [acme@quaco pahole]$ git log --oneline v1.20..
> > ae0b7dde1fd50b12 (HEAD -> master, origin/tmp.master, origin/master, origin/HEAD, github/master, five/master, acme.korg/tmp.master, acme.korg/master) dwarf_loader: Handle DWARF5 DW_OP_addrx properly
> > 9adb014930f31c66 dwarf_loader: Handle subprogram ret type with abstract_origin properly
> > 5752d1951d081a80 dwarf_loader: Check .notes section for LTO build info
> > 209e45424ff4a22d dwarf_loader: Check .debug_abbrev for cross-CU references
> > 39227909db3cc2c2 dwarf_loader: Permit merging all DWARF CU's for clang LTO built binary
> > 763475ca1101ccfe dwarf_loader: Factor out common code to initialize a cu
> > d0d3fbd4744953e8 dwarf_loader: Permit a flexible HASHTAGS__BITS
> > ffe0ef4d73906c18 btf: Add --btf_gen_all flag
> > de708b33114d42c2 btf: Add support for the floating-point types
> > 4b7f8c04d009942b fprintf: Honour conf_fprintf.hex when printing enumerations
> > f2889ff163726336 Avoid warning when building with NDEBUG
> > 8e1f8c904e303d5d btf_encoder: Match ftrace addresses within ELF functions
> > 9fecc77ed82d429f dwarf_loader: Use a better hashing function, from libbpf
> > 0125de3a4c055cdf btf_encoder: Funnel ELF error reporting through a macro
> > 7d8e829f636f47ab btf_encoder: Sanitize non-regular int base type
> > [acme@quaco pahole]$
> > 
> > - Arnaldo
> > 
> 

-- 

- Arnaldo

Re: [PATCH dwarves] dwarf_loader: handle subprogram ret type with abstract_origin properly

[Yonghong Song]

On 4/7/21 8:37 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Apr 07, 2021 at 07:54:26AM -0700, Yonghong Song escreveu:
>> On 4/2/21 11:08 AM, Arnaldo wrote:
>>> On April 2, 2021 2:42:21 PM GMT-03:00, Yonghong Song <yhs@fb.com> wrote:
>>>> On 4/2/21 10:23 AM, Yonghong Song wrote:
>>> :> Thanks. I checked out the branch and did some testing with latest
>>>> clang
>>>>> trunk (just pulled in).
>>>>>
>>>>> With kernel LTO note support, I tested gcc non-lto, and llvm-lto
>>>> mode,
>>>>> it works fine.
>>>>>
>>>>> Without kernel LTO note support, I tested
>>>>>      gcc non-lto  <=== ok
>>>>>      llvm non-lto  <=== not ok
>>>>>      llvm lto     <=== ok
>>>>>
>>>>> Surprisingly llvm non-lto vmlinux had the same "tcp_slow_start"
>>>> issue.
>>>>> Some previous version of clang does not have this issue.
>>>>> I double checked the dwarfdump and it is indeed has the same reason
>>>>> for lto vmlinux. I checked abbrev section and there is no cross-cu
>>>>> references.
>>>>>
>>>>> That means we need to adapt this patch
>>>>>      dwarf_loader: Handle subprogram ret type with abstract_origin
>>>> properly
>>>>> for non merging case as well.
>>>>> The previous patch fixed lto subprogram abstract_origin issue,
>>>>> I will submit a followup patch for this.
>>>>
>>>> Actually, the change is pretty simple,
>>>>
>>>> diff --git a/dwarf_loader.c b/dwarf_loader.c
>>>> index 5dea837..82d7131 100644
>>>> --- a/dwarf_loader.c
>>>> +++ b/dwarf_loader.c
>>>> @@ -2323,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die
>>>> *die, struct cu *cu)
>>>>           int ret = die__process(die, cu);
>>>>           if (ret != 0)
>>>>                   return ret;
>>>> -       return cu__recode_dwarf_types(cu);
>>>> +       ret = cu__recode_dwarf_types(cu);
>>>> +       if (ret != 0)
>>>> +               return ret;
>>>> +
>>>> +       return cu__resolve_func_ret_types(cu);
>>>>    }
>>>>
>>>> Arnaldo, do you just want to fold into previous patches, or
>>>> you want me to submit a new one?
>>>
>>> I can take care of that.
>>
>> Arnaldo, just in case that you missed it, please remember
>> to fold the above changes to the patch:
>>     [PATCH dwarves] dwarf_loader: handle subprogram ret type with
>> abstract_origin properly
>> Thanks!
> 
> Its there, I did it Sunday, IIRC:
> 
> https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?h=tmp.master&id=9adb014930f31c66608fa39a35ccea2daa5586ad
> 
> @@ -2295,7 +2323,11 @@ static int die__process_and_recode(Dwarf_Die *die, struct cu *cu)
>   	int ret = die__process(die, cu);
>   	if (ret != 0)
>   		return ret;
> -	return cu__recode_dwarf_types(cu);
> +	ret = cu__recode_dwarf_types(cu);
> +	if (ret != 0)
> +		return ret;
> +
> +	return cu__resolve_func_ret_types(cu);
>   }
> 
>   static int class_member__cache_byte_size(struct tag *tag, struct cu *cu,
> 
> ----

Thanks! I just saw it in the master branch now. Somehow I did not see
it in tmp.master branch yesterday and I don't know why.

> 
> My latest tests were all with it in place.
> 
> - Arnaldo
>   
>>>
>>> And I think it's time for to look at Jiri's test suite... :-)
>>>
>>> It's a holiday here, so I'll take some time to get to this, hopefully I'll tag 1.21 tomorrow tho.
>>>
>>> Cheers,
>>>
>>> - Arnaldo
>>>
>