[PATCH 0/1] btf_loader support for subprogram linkage

[PATCH 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

First, 

I am a huge fan of pahole (and friends). I volunteered to work on
helping to edit the BTF spec for the IETF and so I've started to look
more deeply at BTF and the tools. 

Second, I hope that what I am offering is being sent to the right place
and is in the right format. I tried to follow what seems to be the
"right thing" by looking at mailing list archives.

This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
linkages. For example,

```
$ cat a.c
static int x() {
  return 5;
}
$ gcc -gbtf -g -O0 -c a.c 
$ ~/code/pahole/build/pfunct -Fbtf --compile a.o
int x(void) /* linkage=static */
{
	return 0;
}
```

Sincerely,
Will

Will Hawkins (1):
  btf_loader: Support linkages for BTF subprograms

 btf_loader.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.45.2

[PATCH 1/1] btf_loader: Support linkages for BTF subprograms

[Will Hawkins]

Handle static and external linkage for BTF subprograms (functions).

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
 btf_loader.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/btf_loader.c b/btf_loader.c
index e0d029a..488c757 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -30,6 +30,9 @@
 #include "dutil.h"
 #include "dwarves.h"
 
+static char btf_linkage_name_static[] = "static";
+static char btf_linkage_name_extern[] = "extern";
+
 static const char *cu__btf_str(struct cu *cu, uint32_t offset)
 {
 	return offset ? btf__str_by_offset(cu->priv, offset) : NULL;
@@ -87,6 +90,11 @@ static int create_new_function(struct cu *cu, const struct btf_type *tp, uint32_
 	// for BTF this is not really the type of the return of the function,
 	// but the prototype, the return type is the one in type_id
 	func->btf = 1;
+	if (BTF_INFO_VLEN(tp->info) == BTF_FUNC_STATIC)
+		func->linkage_name =  btf_linkage_name_static;
+	else if (BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN)
+		func->linkage_name =  btf_linkage_name_extern;
+	func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN;
 	func->proto.tag.tag = DW_TAG_subprogram;
 	func->proto.tag.type = tp->type;
 	func->name = cu__btf_str(cu, tp->name_off);
-- 
2.45.2

Re: [PATCH 1/1] btf_loader: Support linkages for BTF subprograms

[Alan Maguire]

On 23/08/2024 22:28, Will Hawkins wrote:
> Handle static and external linkage for BTF subprograms (functions).
>

This looks good to me Will, thanks! A small nit is that it might be
worth mentioning explicitly in the commit that BTF_FUNC_GLOBAL linkage
is ignored since it does not require a linkage modifier for the
function, but..

> Signed-off-by: Will Hawkins <hawkinsw@obs.cr>

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  btf_loader.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/btf_loader.c b/btf_loader.c
> index e0d029a..488c757 100644
> --- a/btf_loader.c
> +++ b/btf_loader.c
> @@ -30,6 +30,9 @@
>  #include "dutil.h"
>  #include "dwarves.h"
>  
> +static char btf_linkage_name_static[] = "static";
> +static char btf_linkage_name_extern[] = "extern";
> +
>  static const char *cu__btf_str(struct cu *cu, uint32_t offset)
>  {
>  	return offset ? btf__str_by_offset(cu->priv, offset) : NULL;
> @@ -87,6 +90,11 @@ static int create_new_function(struct cu *cu, const struct btf_type *tp, uint32_
>  	// for BTF this is not really the type of the return of the function,
>  	// but the prototype, the return type is the one in type_id
>  	func->btf = 1;
> +	if (BTF_INFO_VLEN(tp->info) == BTF_FUNC_STATIC)
> +		func->linkage_name =  btf_linkage_name_static;
> +	else if (BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN)
> +		func->linkage_name =  btf_linkage_name_extern;
> +	func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN;
>  	func->proto.tag.tag = DW_TAG_subprogram;
>  	func->proto.tag.type = tp->type;
>  	func->name = cu__btf_str(cu, tp->name_off);

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Arnaldo Carvalho de Melo]

On Fri, Aug 23, 2024 at 05:28:30PM -0400, Will Hawkins wrote:
> First, 
> 
> I am a huge fan of pahole (and friends). I volunteered to work on
> helping to edit the BTF spec for the IETF and so I've started to look
> more deeply at BTF and the tools. 
> 
> Second, I hope that what I am offering is being sent to the right place
> and is in the right format. I tried to follow what seems to be the
> "right thing" by looking at mailing list archives.
> 
> This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
> linkages. For example,
> 
> ```
> $ cat a.c
> static int x() {
>   return 5;
> }
> $ gcc -gbtf -g -O0 -c a.c 
> $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> int x(void) /* linkage=static */
> {
> 	return 0;
> }
> ```

So, I'm changing this to become:

  $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
  static int x(void)
  {
  	return 0;
  }

As --compile is supposed to generate compileable code that is as much as
possible from the type information similar to the original code, ok?

Thanks!

- Arnaldo
 
> Sincerely,
> Will
> 
> Will Hawkins (1):
>   btf_loader: Support linkages for BTF subprograms
> 
>  btf_loader.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Arnaldo Carvalho de Melo]

On Mon, Aug 26, 2024 at 11:52:13AM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Aug 23, 2024 at 05:28:30PM -0400, Will Hawkins wrote:
> > First, 
> > 
> > I am a huge fan of pahole (and friends). I volunteered to work on
> > helping to edit the BTF spec for the IETF and so I've started to look
> > more deeply at BTF and the tools. 
> > 
> > Second, I hope that what I am offering is being sent to the right place
> > and is in the right format. I tried to follow what seems to be the
> > "right thing" by looking at mailing list archives.
> > 
> > This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
> > linkages. For example,
> > 
> > ```
> > $ cat a.c
> > static int x() {
> >   return 5;
> > }
> > $ gcc -gbtf -g -O0 -c a.c 
> > $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> > int x(void) /* linkage=static */
> > {
> > 	return 0;
> > }
> > ```
> 
> So, I'm changing this to become:
> 
>   $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
>   static int x(void)
>   {
>   	return 0;
>   }
> 
> As --compile is supposed to generate compileable code that is as much as
> possible from the type information similar to the original code, ok?

I see, your patch isn't the one adding that linkage comment, you're just
collecting it from BTF, I'll then add a followup patch to get to the
output I described.

- Arnaldo

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Arnaldo Carvalho de Melo]

On Mon, Aug 26, 2024 at 11:53:56AM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Aug 26, 2024 at 11:52:13AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Aug 23, 2024 at 05:28:30PM -0400, Will Hawkins wrote:
> > > First, 
> > > 
> > > I am a huge fan of pahole (and friends). I volunteered to work on
> > > helping to edit the BTF spec for the IETF and so I've started to look
> > > more deeply at BTF and the tools. 
> > > 
> > > Second, I hope that what I am offering is being sent to the right place
> > > and is in the right format. I tried to follow what seems to be the
> > > "right thing" by looking at mailing list archives.
> > > 
> > > This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
> > > linkages. For example,
> > > 
> > > ```
> > > $ cat a.c
> > > static int x() {
> > >   return 5;
> > > }
> > > $ gcc -gbtf -g -O0 -c a.c 
> > > $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> > > int x(void) /* linkage=static */
> > > {
> > > 	return 0;
> > > }
> > > ```
> > 
> > So, I'm changing this to become:
> > 
> >   $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> >   static int x(void)
> >   {
> >   	return 0;
> >   }
> > 
> > As --compile is supposed to generate compileable code that is as much as
> > possible from the type information similar to the original code, ok?
> 
> I see, your patch isn't the one adding that linkage comment, you're just
> collecting it from BTF, I'll then add a followup patch to get to the
> output I described.

So I'll looked up what that linkage field comes from and:

commit ce516fb0cf2a3d9fc45c2a9c7ab2cf4bd24965a0
Author: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Date:   Sun Jul 8 19:47:26 2007 -0300

    [LIB]: Support DW_AT_MIPS_linkage_name
    
    Another C++ specific case:
    
    -       class TypeTemplate ByName(const string  &, size_t);
    +       class TypeTemplate ByName(const string  &, size_t); /* linkage=_ZN4ROOT6Reflex12TypeTemplate6ByNameERKSsj */
    
    Signed-off-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>

So its not really "static" or "stern", but the mangled name in languages
such as C++, so I think this patch needs reworking, there is ambiguity
on the "linkage" term, we should use some other term :-\

- Arnaldo

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

On Mon, Aug 26, 2024 at 10:58 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Aug 26, 2024 at 11:53:56AM -0300, Arnaldo Carvalho de Melo wrote:
> > On Mon, Aug 26, 2024 at 11:52:13AM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Fri, Aug 23, 2024 at 05:28:30PM -0400, Will Hawkins wrote:
> > > > First,
> > > >
> > > > I am a huge fan of pahole (and friends). I volunteered to work on
> > > > helping to edit the BTF spec for the IETF and so I've started to look
> > > > more deeply at BTF and the tools.
> > > >
> > > > Second, I hope that what I am offering is being sent to the right place
> > > > and is in the right format. I tried to follow what seems to be the
> > > > "right thing" by looking at mailing list archives.
> > > >
> > > > This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
> > > > linkages. For example,
> > > >
> > > > ```
> > > > $ cat a.c
> > > > static int x() {
> > > >   return 5;
> > > > }
> > > > $ gcc -gbtf -g -O0 -c a.c
> > > > $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> > > > int x(void) /* linkage=static */
> > > > {
> > > >   return 0;
> > > > }
> > > > ```
> > >
> > > So, I'm changing this to become:
> > >
> > >   $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> > >   static int x(void)
> > >   {
> > >     return 0;
> > >   }
> > >
> > > As --compile is supposed to generate compileable code that is as much as
> > > possible from the type information similar to the original code, ok?
> >
> > I see, your patch isn't the one adding that linkage comment, you're just
> > collecting it from BTF, I'll then add a followup patch to get to the
> > output I described.
>
> So I'll looked up what that linkage field comes from and:
>
> commit ce516fb0cf2a3d9fc45c2a9c7ab2cf4bd24965a0
> Author: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> Date:   Sun Jul 8 19:47:26 2007 -0300
>
>     [LIB]: Support DW_AT_MIPS_linkage_name
>
>     Another C++ specific case:
>
>     -       class TypeTemplate ByName(const string  &, size_t);
>     +       class TypeTemplate ByName(const string  &, size_t); /* linkage=_ZN4ROOT6Reflex12TypeTemplate6ByNameERKSsj */
>
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
>
> So its not really "static" or "stern", but the mangled name in languages
> such as C++, so I think this patch needs reworking, there is ambiguity
> on the "linkage" term, we should use some other term :-\

Completely understand! I will rework it and propose something different.

I am just thankful that you are so welcoming of the contribution! Your
enthusiasm has made a contributor for life!

Will


>
> - Arnaldo

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Arnaldo Carvalho de Melo]

On Mon, Aug 26, 2024 at 12:36:45PM -0400, Will Hawkins wrote:
> On Mon, Aug 26, 2024 at 10:58 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > commit ce516fb0cf2a3d9fc45c2a9c7ab2cf4bd24965a0
> > Author: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> > Date:   Sun Jul 8 19:47:26 2007 -0300

> >     [LIB]: Support DW_AT_MIPS_linkage_name

> >     Another C++ specific case:

> >     -       class TypeTemplate ByName(const string  &, size_t);
> >     +       class TypeTemplate ByName(const string  &, size_t); /* linkage=_ZN4ROOT6Reflex12TypeTemplate6ByNameERKSsj */

> >     Signed-off-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>

> > So its not really "static" or "stern", but the mangled name in languages

oops, "extern"

> > such as C++, so I think this patch needs reworking, there is ambiguity
> > on the "linkage" term, we should use some other term :-\
> 
> Completely understand! I will rework it and propose something different.
> 
> I am just thankful that you are so welcoming of the contribution! Your
> enthusiasm has made a contributor for life!

Another recommendation, please don't point the field to strings like
"extern" or "static", as DWARF is not language specific, pahole manages
to handle C mostly, C++ to a great degree but I also used it with Go,
Rust.

We want to make it more language agnostic, so while the "linkage" as a
mangled name is not language specific, "static" or "extern" may be.

So just keep it using the DWARF constant/enumeration, so that at
dwarves_fprintf.c time we can, based on the language encoded in the
debugging info (for BTF we assume C until we introduce some way to
encode Rust (The kernel can be written using Rust, so we're bound to
have something in that direction introduced in BTF at some point)).

- Arnaldo

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

On Mon, Aug 26, 2024 at 4:00 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Aug 26, 2024 at 12:36:45PM -0400, Will Hawkins wrote:
> > On Mon, Aug 26, 2024 at 10:58 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
> > > commit ce516fb0cf2a3d9fc45c2a9c7ab2cf4bd24965a0
> > > Author: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
> > > Date:   Sun Jul 8 19:47:26 2007 -0300
>
> > >     [LIB]: Support DW_AT_MIPS_linkage_name
>
> > >     Another C++ specific case:
>
> > >     -       class TypeTemplate ByName(const string  &, size_t);
> > >     +       class TypeTemplate ByName(const string  &, size_t); /* linkage=_ZN4ROOT6Reflex12TypeTemplate6ByNameERKSsj */
>
> > >     Signed-off-by: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
>
> > > So its not really "static" or "stern", but the mangled name in languages
>
> oops, "extern"
>
> > > such as C++, so I think this patch needs reworking, there is ambiguity
> > > on the "linkage" term, we should use some other term :-\
> >
> > Completely understand! I will rework it and propose something different.
> >
> > I am just thankful that you are so welcoming of the contribution! Your
> > enthusiasm has made a contributor for life!
>
> Another recommendation, please don't point the field to strings like
> "extern" or "static", as DWARF is not language specific, pahole manages
> to handle C mostly, C++ to a great degree but I also used it with Go,
> Rust.
>
> We want to make it more language agnostic, so while the "linkage" as a
> mangled name is not language specific, "static" or "extern" may be.
>
> So just keep it using the DWARF constant/enumeration, so that at
> dwarves_fprintf.c time we can, based on the language encoded in the
> debugging info (for BTF we assume C until we introduce some way to
> encode Rust (The kernel can be written using Rust, so we're bound to
> have something in that direction introduced in BTF at some point)).

100% understood!

Thank you for the feedback!
Will


>
> - Arnaldo

Re: [PATCH 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

On Mon, Aug 26, 2024 at 10:52 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Fri, Aug 23, 2024 at 05:28:30PM -0400, Will Hawkins wrote:
> > First,
> >
> > I am a huge fan of pahole (and friends). I volunteered to work on
> > helping to edit the BTF spec for the IETF and so I've started to look
> > more deeply at BTF and the tools.
> >
> > Second, I hope that what I am offering is being sent to the right place
> > and is in the right format. I tried to follow what seems to be the
> > "right thing" by looking at mailing list archives.
> >
> > This patch add supports to the btf_loader for subprogram (BTF_KIND_FUNC)
> > linkages. For example,
> >
> > ```
> > $ cat a.c
> > static int x() {
> >   return 5;
> > }
> > $ gcc -gbtf -g -O0 -c a.c
> > $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
> > int x(void) /* linkage=static */
> > {
> >       return 0;
> > }
> > ```
>
> So, I'm changing this to become:
>
>   $ ~/code/pahole/build/pfunct -Fbtf --compile a.o
>   static int x(void)
>   {
>         return 0;
>   }
>
> As --compile is supposed to generate compileable code that is as much as
> possible from the type information similar to the original code, ok?
>
> Thanks!

I agree with this 100%. I *almost* made that change, but didn't want
to overstep my bounds!

Will


>
> - Arnaldo
>
> > Sincerely,
> > Will
> >
> > Will Hawkins (1):
> >   btf_loader: Support linkages for BTF subprograms
> >
> >  btf_loader.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > --
> > 2.45.2
> >
> >

[PATCH v2 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

Thank you for the feedback on my first attempt. I believe that this
attempt is better. Given the feedback, I

0. Removed the incorrect use of linkage_name field;
1. Tried to maintain the semantics of DWARF as much as possible;
2. Reworked how BTF information is loaded into a new function based on
my understanding of the semantics of BTF_FUNC_STATIC, BTF_FUNC_GLOBAL,
and BTF_FUNC_EXTERN; and
3. Modified (slightly) the way functions are printed in pfunc to show
linkage where appropriate (based, in particular, on 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45153#c6.

I saw that Arnaldo and Namhyung have made similar changes to the
printing code so I hope that I am not stepping on their work (sorry if I
am!).

Again, I appreciate the feedback and hope that I was able to address it
with this v2.

Sincerely,

Will

Will Hawkins (1):
  btf_loader: Support linkages for BTF subprograms

 btf_loader.c |  4 +++-
 dwarves.h    |  2 +-
 pfunct.c     | 15 ++++++++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.45.2

Re: [PATCH v2 0/1] btf_loader support for subprogram linkage

[Arnaldo Carvalho de Melo]

On Tue, Aug 27, 2024 at 12:09:07AM -0400, Will Hawkins wrote:
> Thank you for the feedback on my first attempt. I believe that this
> attempt is better. Given the feedback, I
> 
> 0. Removed the incorrect use of linkage_name field;
> 1. Tried to maintain the semantics of DWARF as much as possible;
> 2. Reworked how BTF information is loaded into a new function based on
> my understanding of the semantics of BTF_FUNC_STATIC, BTF_FUNC_GLOBAL,
> and BTF_FUNC_EXTERN; and
> 3. Modified (slightly) the way functions are printed in pfunc to show
> linkage where appropriate (based, in particular, on 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45153#c6.
> 
> I saw that Arnaldo and Namhyung have made similar changes to the
> printing code so I hope that I am not stepping on their work (sorry if I
> am!).
> 
> Again, I appreciate the feedback and hope that I was able to address it
> with this v2.

So, I broke it down into two patches, the first for the BTF loader and
then a second patch for pfunct to use that info, but:

⬢[acme@toolbox pahole]$ readelf -wi cases/btf_linkages/linkage.o
Contents of the .debug_info section:

  Compilation Unit @ offset 0:
   Length:        0x4e (32-bit)
   Version:       5
   Unit Type:     DW_UT_compile (1)
   Abbrev Offset: 0
   Pointer Size:  8
 <0><c>: Abbrev Number: 1 (DW_TAG_compile_unit)
    <d>   DW_AT_producer    : (indirect string, offset: 0x5): GNU C17 14.1.1 20240701 (Red Hat 14.1.1-7) -mtune=generic -march=x86-64 -gbtf -g -O0
    <11>   DW_AT_language    : 29	(C11)
    <12>   DW_AT_name        : (indirect line string, offset: 0): linkage.c
    <16>   DW_AT_comp_dir    : (indirect line string, offset: 0xa): /home/acme/git/pahole/cases/btf_linkages
    <1a>   DW_AT_low_pc      : 0
    <22>   DW_AT_high_pc     : 0xb
    <2a>   DW_AT_stmt_list   : 0
 <1><2e>: Abbrev Number: 2 (DW_TAG_subprogram)
    <2f>   DW_AT_name        : x
    <31>   DW_AT_decl_file   : 1
    <32>   DW_AT_decl_line   : 6
    <33>   DW_AT_decl_column : 12
    <34>   DW_AT_type        : <0x4a>
    <38>   DW_AT_low_pc      : 0
    <40>   DW_AT_high_pc     : 0xb
    <48>   DW_AT_frame_base  : 1 byte block: 9c 	(DW_OP_call_frame_cfa)
    <4a>   DW_AT_call_all_calls: 1
 <1><4a>: Abbrev Number: 3 (DW_TAG_base_type)
    <4b>   DW_AT_byte_size   : 4
    <4c>   DW_AT_encoding    : 5	(signed)
    <4d>   DW_AT_name        : int
 <1><51>: Abbrev Number: 0

⬢[acme@toolbox pahole]$

In DWARF we don't have that DW_AT_external, so the simplified second
patch:

⬢[acme@toolbox pahole]$ git diff
diff --git a/pfunct.c b/pfunct.c
index bc3026b384284cb4..9cf61713354d178e 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -377,6 +377,9 @@ static void function__show(struct function *func, struct cu *cu)
        if (fstats && fstats->printed)
                return;
 
+       if (!func->external)
+               fprintf(stdout, "static ");
+
        if (expand_types)
                function__emit_type_definitions(func, cu, stdout);
        tag__fprintf(tag, cu, &conf, stdout);
⬢[acme@toolbox pahole]$

Would print as static, but it isn't...

⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c
// https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@obs.cr/T/#u
// Will Hawkins
// $ gcc -gbtf -g -O0 -c linkage.c
// $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o

static int x() {
	return 5;
}
⬢[acme@toolbox pahole]$

⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
int x(void);
⬢[acme@toolbox pahole]$


So, if instead I do it at:

⬢[acme@toolbox pahole]$ git diff
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index ffbe6fbd3f5c405a..9a8bb5f15c88125a 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -1406,6 +1406,9 @@ static size_t function__fprintf(const struct tag *tag, const struct cu *cu,
        size_t printed = 0;
        bool inlined = !conf->strip_inline && function__declared_inline(func);
 
+       if (!func->external)
+               printed += fprintf(fp, "%s ", "static");
+
        if (tag->attribute)
                printed += fprintf(fp, "%s ", tag->attribute);
 
⬢[acme@toolbox pahole]$

Then we seem to get what we want:

⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
static int x(void);
⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o
static int x(void)
{
	return 0;
}

⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o
static int x(void);
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
static int x(void);
⬢[acme@toolbox pahole]$ 

And if we add a non-static function, it also works for both DWARF and
BTF when generated by gcc:

⬢[acme@toolbox btf_linkages]$ gcc -gbtf -g -O0 -c linkage.c
⬢[acme@toolbox btf_linkages]$ cd -
/home/acme/git/pahole
⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c
// https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@obs.cr/T/#u
// Will Hawkins
// $ gcc -gbtf -g -O0 -c linkage.c
// $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o

static int x() {
	return 5;
}

int y() {
	return 4;
}
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
static int x(void);
int y(void);
⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o
static int x(void);
int y(void);
⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o
int y(void)
{
	return 0;
}

static int x(void)
{
	return 0;
}

⬢[acme@toolbox pahole]$ pfunct --compile -F dwarf cases/btf_linkages/linkage.o
int y(void)
{
}

static int x(void)
{
}

⬢[acme@toolbox pahole]$

But, for a vmlinux I have laying around it seems we're not encoding that
with pahole:

⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | head
static void clean_cache_range(void * addr, size_t size);
static void arch_wb_cache_pmem(void * addr, size_t size);
static long int __copy_user_flushcache(void * dst, const void  * src, unsigned int size);
static void __memcpy_flushcache(void * _dst, const void  * _src, size_t size);
static long unsigned int copy_from_user_nmi(void * to, const void  * from, long unsigned int n);
static void call_depth_return_thunk(void);
static void entry_untrain_ret(void);
static void retbleed_untrain_ret(void);
static void srso_untrain_ret(void);
static void srso_alias_safe_ret(void);
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | wc -l
59015
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | wc -l
59015
⬢[acme@toolbox pahole]$

⬢[acme@toolbox pahole]$ pahole --btf_encode -j vmlinux 
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | wc -l
59650
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | wc -l
59650
⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep -v ^static
⬢[acme@toolbox pahole]$

So before we change function__fprintf() we need to investigate this
other part: pahole seemingly not encoding:

+       func->declaration = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN;
+       func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_GLOBAL;

The thing is we want to release 1.28, so lets leave this for after that,
ok.

- Arnaldo

Re: [PATCH v2 0/1] btf_loader support for subprogram linkage

[Will Hawkins]

On Tue, Aug 27, 2024 at 10:37 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, Aug 27, 2024 at 12:09:07AM -0400, Will Hawkins wrote:
> > Thank you for the feedback on my first attempt. I believe that this
> > attempt is better. Given the feedback, I
> >
> > 0. Removed the incorrect use of linkage_name field;
> > 1. Tried to maintain the semantics of DWARF as much as possible;
> > 2. Reworked how BTF information is loaded into a new function based on
> > my understanding of the semantics of BTF_FUNC_STATIC, BTF_FUNC_GLOBAL,
> > and BTF_FUNC_EXTERN; and
> > 3. Modified (slightly) the way functions are printed in pfunc to show
> > linkage where appropriate (based, in particular, on
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=45153#c6.
> >
> > I saw that Arnaldo and Namhyung have made similar changes to the
> > printing code so I hope that I am not stepping on their work (sorry if I
> > am!).
> >
> > Again, I appreciate the feedback and hope that I was able to address it
> > with this v2.
>
> So, I broke it down into two patches, the first for the BTF loader and
> then a second patch for pfunct to use that info, but:
>
> ⬢[acme@toolbox pahole]$ readelf -wi cases/btf_linkages/linkage.o
> Contents of the .debug_info section:
>
>   Compilation Unit @ offset 0:
>    Length:        0x4e (32-bit)
>    Version:       5
>    Unit Type:     DW_UT_compile (1)
>    Abbrev Offset: 0
>    Pointer Size:  8
>  <0><c>: Abbrev Number: 1 (DW_TAG_compile_unit)
>     <d>   DW_AT_producer    : (indirect string, offset: 0x5): GNU C17 14.1.1 20240701 (Red Hat 14.1.1-7) -mtune=generic -march=x86-64 -gbtf -g -O0
>     <11>   DW_AT_language    : 29       (C11)
>     <12>   DW_AT_name        : (indirect line string, offset: 0): linkage.c
>     <16>   DW_AT_comp_dir    : (indirect line string, offset: 0xa): /home/acme/git/pahole/cases/btf_linkages
>     <1a>   DW_AT_low_pc      : 0
>     <22>   DW_AT_high_pc     : 0xb
>     <2a>   DW_AT_stmt_list   : 0
>  <1><2e>: Abbrev Number: 2 (DW_TAG_subprogram)
>     <2f>   DW_AT_name        : x
>     <31>   DW_AT_decl_file   : 1
>     <32>   DW_AT_decl_line   : 6
>     <33>   DW_AT_decl_column : 12
>     <34>   DW_AT_type        : <0x4a>
>     <38>   DW_AT_low_pc      : 0
>     <40>   DW_AT_high_pc     : 0xb
>     <48>   DW_AT_frame_base  : 1 byte block: 9c         (DW_OP_call_frame_cfa)
>     <4a>   DW_AT_call_all_calls: 1
>  <1><4a>: Abbrev Number: 3 (DW_TAG_base_type)
>     <4b>   DW_AT_byte_size   : 4
>     <4c>   DW_AT_encoding    : 5        (signed)
>     <4d>   DW_AT_name        : int
>  <1><51>: Abbrev Number: 0
>
> ⬢[acme@toolbox pahole]$
>
> In DWARF we don't have that DW_AT_external, so the simplified second
> patch:
>
> ⬢[acme@toolbox pahole]$ git diff
> diff --git a/pfunct.c b/pfunct.c
> index bc3026b384284cb4..9cf61713354d178e 100644
> --- a/pfunct.c
> +++ b/pfunct.c
> @@ -377,6 +377,9 @@ static void function__show(struct function *func, struct cu *cu)
>         if (fstats && fstats->printed)
>                 return;
>
> +       if (!func->external)
> +               fprintf(stdout, "static ");
> +
>         if (expand_types)
>                 function__emit_type_definitions(func, cu, stdout);
>         tag__fprintf(tag, cu, &conf, stdout);
> ⬢[acme@toolbox pahole]$
>
> Would print as static, but it isn't...
>
> ⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c
> // https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@obs.cr/T/#u
> // Will Hawkins
> // $ gcc -gbtf -g -O0 -c linkage.c
> // $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o
>
> static int x() {
>         return 5;
> }
> ⬢[acme@toolbox pahole]$
>
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
> int x(void);
> ⬢[acme@toolbox pahole]$
>

Sorry for the trouble! I *thought* that I tested cases like these and
got results that seemed reasonable. Clearly what you got does not seem
to work. So, I will go through and test using a few different cases,
make sure that I get the right answer, then confirm with you in a
follow-up patch.

(See below)

>
> So, if instead I do it at:
>
> ⬢[acme@toolbox pahole]$ git diff
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index ffbe6fbd3f5c405a..9a8bb5f15c88125a 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -1406,6 +1406,9 @@ static size_t function__fprintf(const struct tag *tag, const struct cu *cu,
>         size_t printed = 0;
>         bool inlined = !conf->strip_inline && function__declared_inline(func);
>
> +       if (!func->external)
> +               printed += fprintf(fp, "%s ", "static");
> +
>         if (tag->attribute)
>                 printed += fprintf(fp, "%s ", tag->attribute);
>
> ⬢[acme@toolbox pahole]$
>
> Then we seem to get what we want:
>
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
> static int x(void);
> ⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o
> static int x(void)
> {
>         return 0;
> }
>
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o
> static int x(void);
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
> static int x(void);
> ⬢[acme@toolbox pahole]$
>
> And if we add a non-static function, it also works for both DWARF and
> BTF when generated by gcc:
>
> ⬢[acme@toolbox btf_linkages]$ gcc -gbtf -g -O0 -c linkage.c
> ⬢[acme@toolbox btf_linkages]$ cd -
> /home/acme/git/pahole
> ⬢[acme@toolbox pahole]$ cat cases/btf_linkages/linkage.c
> // https://lore.kernel.org/all/20240823212835.1078351-1-hawkinsw@obs.cr/T/#u
> // Will Hawkins
> // $ gcc -gbtf -g -O0 -c linkage.c
> // $ ~/code/pahole/build/pfunct -Fbtf --compile linkage.o
>
> static int x() {
>         return 5;
> }
>
> int y() {
>         return 4;
> }
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf cases/btf_linkages/linkage.o
> static int x(void);
> int y(void);
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F dwarf cases/btf_linkages/linkage.o
> static int x(void);
> int y(void);
> ⬢[acme@toolbox pahole]$ pfunct --compile -F btf cases/btf_linkages/linkage.o
> int y(void)
> {
>         return 0;
> }
>
> static int x(void)
> {
>         return 0;
> }
>
> ⬢[acme@toolbox pahole]$ pfunct --compile -F dwarf cases/btf_linkages/linkage.o
> int y(void)
> {
> }
>
> static int x(void)
> {
> }
>
> ⬢[acme@toolbox pahole]$
>
> But, for a vmlinux I have laying around it seems we're not encoding that
> with pahole:
>
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | head
> static void clean_cache_range(void * addr, size_t size);
> static void arch_wb_cache_pmem(void * addr, size_t size);
> static long int __copy_user_flushcache(void * dst, const void  * src, unsigned int size);
> static void __memcpy_flushcache(void * _dst, const void  * _src, size_t size);
> static long unsigned int copy_from_user_nmi(void * to, const void  * from, long unsigned int n);
> static void call_depth_return_thunk(void);
> static void entry_untrain_ret(void);
> static void retbleed_untrain_ret(void);
> static void srso_untrain_ret(void);
> static void srso_alias_safe_ret(void);
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | wc -l
> 59015
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | wc -l
> 59015
> ⬢[acme@toolbox pahole]$
>
> ⬢[acme@toolbox pahole]$ pahole --btf_encode -j vmlinux
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | wc -l
> 59650
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep ^static | wc -l
> 59650
> ⬢[acme@toolbox pahole]$ pfunct --prototype -F btf vmlinux  | grep -v ^static
> ⬢[acme@toolbox pahole]$
>
> So before we change function__fprintf() we need to investigate this
> other part: pahole seemingly not encoding:
>
> +       func->declaration = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN;
> +       func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_GLOBAL;
>
> The thing is we want to release 1.28, so lets leave this for after that,
> ok.

Sorry -- it took me a few reads through your thorough email to
understand. I appreciate your willingness to engage with me on this
patch! If I understand correctly, you are saying that pahole may not
be encoding the BTF_FUNC* properly. So, we will need to debug that at
the same time as we add this feature.

If that is the case, then I will start to work on that!

If possible, would you be willing to send around that "bad" vmlinux?
(You can send directly to me -- I know that they are far from small).
That would help me debug and recreate what you are seeing.

Thank you again for engaging with me on this effort! I really, really
appreciate your openness! As I said, I am a huge fan of your work (I
really enjoyed your talk at DevConf in 2021)!

Will

>
> - Arnaldo

[PATCH v2 1/1] btf_loader: Support linkages for BTF subprograms

[Will Hawkins]

Handle static and external linkage for BTF subprograms (functions).

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
 btf_loader.c |  4 +++-
 dwarves.h    |  2 +-
 pfunct.c     | 15 ++++++++++++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/btf_loader.c b/btf_loader.c
index e0d029a..16962f9 100644
--- a/btf_loader.c
+++ b/btf_loader.c
@@ -85,8 +85,10 @@ static int create_new_function(struct cu *cu, const struct btf_type *tp, uint32_
 		return -ENOMEM;
 
 	// for BTF this is not really the type of the return of the function,
-	// but the prototype, the return type is the one in type_id
+	// but the prototype, the return type is the one in type_id.
 	func->btf = 1;
+	func->declaration = BTF_INFO_VLEN(tp->info) == BTF_FUNC_EXTERN;
+	func->external = BTF_INFO_VLEN(tp->info) == BTF_FUNC_GLOBAL;
 	func->proto.tag.tag = DW_TAG_subprogram;
 	func->proto.tag.type = tp->type;
 	func->name = cu__btf_str(cu, tp->name_off);
diff --git a/dwarves.h b/dwarves.h
index f5ae79f..c6d2b63 100644
--- a/dwarves.h
+++ b/dwarves.h
@@ -983,7 +983,7 @@ struct function {
 	uint16_t	 cu_total_nr_inline_expansions;
 	uint8_t		 inlined:2;
 	uint8_t		 abstract_origin:1;
-	uint8_t		 external:1;
+	uint8_t		 external:1; /* DW_AT_external */
 	uint8_t		 accessibility:2; /* DW_ACCESS_{public,protected,private} */
 	uint8_t		 virtuality:2; /* DW_VIRTUALITY_{none,virtual,pure_virtual} */
 	uint8_t		 declaration:1;
diff --git a/pfunct.c b/pfunct.c
index 938dd72..725cc2e 100644
--- a/pfunct.c
+++ b/pfunct.c
@@ -366,9 +366,22 @@ static void function__show(struct function *func, struct cu *cu)
 {
 	struct tag *tag = function__tag(func);
 
-	if (func->abstract_origin || func->external)
+	if (func->abstract_origin)
 		return;
 
+	// If this function is more than a declaration, then be
+	// explicit about its linkage. For dwarf, external comes
+	// from DW_AT_external which, when combined with whether
+	// the DIE represents a defined subprogram, determines
+	// the linkage. BTF maintains these semantics when it reads
+	// type information.
+	if (!func->declaration) {
+		if (func->external)
+			fprintf(stdout, "extern ");
+		else
+			fprintf(stdout, "static ");
+	}
+
 	if (expand_types)
 		function__emit_type_definitions(func, cu, stdout);
 	tag__fprintf(tag, cu, &conf, stdout);
-- 
2.45.2