[PATCH 0/3] btf: fix BTF for extern items [PR106773]

[PATCH 0/3] btf: fix BTF for extern items [PR106773]

[David Faust]

Hi,

This series fixes the issues reported in target/PR106773. I decided to
split it into three commits, as there are ultimately three distinct
issues and fixes. See each patch for details.

Tested on bpf-unknown-none and x86_64-linux-gnu, no known regressions.

OK to push?
Thanks.

David Faust (3):
  btf: add 'extern' linkage for variables [PR106773]
  btf: fix 'extern const void' variables [PR106773]
  btf: correct generation for extern funcs [PR106773]

 gcc/btfout.cc                                 | 182 +++++++++++++-----
 .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 +++
 .../gcc.dg/debug/btf/btf-function-6.c         |  19 ++
 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c |  25 +++
 .../gcc.dg/debug/btf/btf-variables-4.c        |  24 +++
 include/btf.h                                 |  11 +-
 6 files changed, 237 insertions(+), 52 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c

-- 
2.38.1

[PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]

[David Faust]

Add support for the 'extern' linkage value for BTF_KIND_VAR records,
which is used for variables declared as extern in the source file.

	PR target/106773

gcc/

	* btfout.cc (BTF_LINKAGE_STATIC): New define.
	(BTF_LINKAGE_GLOBAL): Likewise.
	(BTF_LINKAGE_EXTERN): Likewise.
	(btf_collect_datasec): Mark extern variables as such.
	(btf_asm_varent): Accomodate 'extern' linkage.

gcc/testsuite/

	* gcc.dg/debug/btf/btf-variables-4.c: New test.

include/

	* btf.h (struct btf_var): Update comment to note 'extern' linkage.
---
 gcc/btfout.cc                                 |  9 ++++++-
 .../gcc.dg/debug/btf/btf-variables-4.c        | 24 +++++++++++++++++++
 include/btf.h                                 |  2 +-
 3 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c

diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index aef9fd70a28..a1c6266a7db 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
 
 #define BTF_INVALID_TYPEID 0xFFFFFFFF
 
+#define BTF_LINKAGE_STATIC 0
+#define BTF_LINKAGE_GLOBAL 1
+#define BTF_LINKAGE_EXTERN 2
+
 /* Mapping of CTF variables to the IDs they will be assigned when they are
    converted to BTF_KIND_VAR type records. Strictly accounts for the index
    from the start of the variable type entries, does not include the number
@@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
 	continue;
 
       const char *section_name = node->get_section ();
+      /* Mark extern variables.  */
+      if (DECL_EXTERNAL (node->decl))
+	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
 
       if (section_name == NULL)
 	{
@@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
   dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
   dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
   dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
-  dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
+  dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
 }
 
 /* Asm'out a member description following a BTF_KIND_STRUCT or
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
new file mode 100644
index 00000000000..d77600bae1c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
@@ -0,0 +1,24 @@
+/* Test BTF generation for extern variables.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect 4 variables.  */
+/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
+
+/* 2 extern, 1 global, 1 static.  */
+/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
+
+extern int a;
+extern const int b;
+int c;
+static const int d = 5;
+
+int foo (int x)
+{
+  c = a + b + x;
+
+  return c + d;
+}
diff --git a/include/btf.h b/include/btf.h
index eba67f9d599..9a757ce5bc9 100644
--- a/include/btf.h
+++ b/include/btf.h
@@ -182,7 +182,7 @@ struct btf_param
    information about the variable.  */
 struct btf_var
 {
-  uint32_t linkage;	/* Currently only 0=static or 1=global.  */
+  uint32_t linkage;	/* 0=static, 1=global, 2=extern.  */
 };
 
 /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
-- 
2.38.1

Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]

[Indu Bhagat]

Hi David,

On 12/7/22 12:57, David Faust wrote:
> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
> which is used for variables declared as extern in the source file.
> 
> 	PR target/106773
> 
> gcc/
> 
> 	* btfout.cc (BTF_LINKAGE_STATIC): New define.
> 	(BTF_LINKAGE_GLOBAL): Likewise.
> 	(BTF_LINKAGE_EXTERN): Likewise.
> 	(btf_collect_datasec): Mark extern variables as such.
> 	(btf_asm_varent): Accomodate 'extern' linkage.
> 
> gcc/testsuite/
> 
> 	* gcc.dg/debug/btf/btf-variables-4.c: New test.
> 
> include/
> 
> 	* btf.h (struct btf_var): Update comment to note 'extern' linkage.
> ---
>   gcc/btfout.cc                                 |  9 ++++++-
>   .../gcc.dg/debug/btf/btf-variables-4.c        | 24 +++++++++++++++++++
>   include/btf.h                                 |  2 +-
>   3 files changed, 33 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index aef9fd70a28..a1c6266a7db 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>   
>   #define BTF_INVALID_TYPEID 0xFFFFFFFF
>   
> +#define BTF_LINKAGE_STATIC 0
> +#define BTF_LINKAGE_GLOBAL 1
> +#define BTF_LINKAGE_EXTERN 2
> +

I was about to suggest to rename these to use the same name as used in 
the kernel btf.h. What is used there is:
         BTF_VAR_STATIC = 0,
         BTF_VAR_GLOBAL_ALLOCATED = 1,
         BTF_VAR_GLOBAL_EXTERN = 2,

But after looking at the Patch 3/3, I see you reuse these definitions 
for functions as well. I just find the names confusing on the first look 
- "BTF_LINKAGE_STATIC".

Naming aside, what do you think about adding the defines to 
include/btf.h instead ?

>   /* Mapping of CTF variables to the IDs they will be assigned when they are
>      converted to BTF_KIND_VAR type records. Strictly accounts for the index
>      from the start of the variable type entries, does not include the number
> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
>   	continue;
>   
>         const char *section_name = node->get_section ();
> +      /* Mark extern variables.  */
> +      if (DECL_EXTERNAL (node->decl))
> +	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>   

This made me think about the following case.

extern const char a[];
const char a[] = "foo";

What is the expected BTF for this? Since BTF can differentiate between 
the non-defining extern variable declaration, I expected to see two 
variables with different "linkage". At this time I see, two variables 
with global linkage but different types:

         .long   0xe000000       # btv_info
         .long   0x4     # btv_type
         .long   0x1     # btv_linkage
         .long   0x1f    # btv_name
         .long   0xe000000       # btv_info
         .long   0x7     # btv_type
         .long   0x1     # btv_linkage
         .long   0x60    # btt_name

>         if (section_name == NULL)
>   	{
> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
>     dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
>     dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
>     dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
> -  dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
> +  dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
>   }
>   
>   /* Asm'out a member description following a BTF_KIND_STRUCT or
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
> new file mode 100644
> index 00000000000..d77600bae1c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
> @@ -0,0 +1,24 @@
> +/* Test BTF generation for extern variables.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect 4 variables.  */
> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
> +
> +/* 2 extern, 1 global, 1 static.  */
> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
> +
> +extern int a;
> +extern const int b;
> +int c;
> +static const int d = 5;
> +
> +int foo (int x)
> +{
> +  c = a + b + x;
> +
> +  return c + d;
> +}
> diff --git a/include/btf.h b/include/btf.h
> index eba67f9d599..9a757ce5bc9 100644
> --- a/include/btf.h
> +++ b/include/btf.h
> @@ -182,7 +182,7 @@ struct btf_param
>      information about the variable.  */
>   struct btf_var
>   {
> -  uint32_t linkage;	/* Currently only 0=static or 1=global.  */
> +  uint32_t linkage;	/* 0=static, 1=global, 2=extern.  */
>   };
>   
>   /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,

Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]

[David Faust]

On 12/8/22 22:55, Indu Bhagat wrote:
> Hi David,
> 
> On 12/7/22 12:57, David Faust wrote:
>> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
>> which is used for variables declared as extern in the source file.
>>
>> 	PR target/106773
>>
>> gcc/
>>
>> 	* btfout.cc (BTF_LINKAGE_STATIC): New define.
>> 	(BTF_LINKAGE_GLOBAL): Likewise.
>> 	(BTF_LINKAGE_EXTERN): Likewise.
>> 	(btf_collect_datasec): Mark extern variables as such.
>> 	(btf_asm_varent): Accomodate 'extern' linkage.
>>
>> gcc/testsuite/
>>
>> 	* gcc.dg/debug/btf/btf-variables-4.c: New test.
>>
>> include/
>>
>> 	* btf.h (struct btf_var): Update comment to note 'extern' linkage.
>> ---
>>   gcc/btfout.cc                                 |  9 ++++++-
>>   .../gcc.dg/debug/btf/btf-variables-4.c        | 24 +++++++++++++++++++
>>   include/btf.h                                 |  2 +-
>>   3 files changed, 33 insertions(+), 2 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index aef9fd70a28..a1c6266a7db 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>>   
>>   #define BTF_INVALID_TYPEID 0xFFFFFFFF
>>   
>> +#define BTF_LINKAGE_STATIC 0
>> +#define BTF_LINKAGE_GLOBAL 1
>> +#define BTF_LINKAGE_EXTERN 2
>> +
> 
> I was about to suggest to rename these to use the same name as used in 
> the kernel btf.h. What is used there is:
>          BTF_VAR_STATIC = 0,
>          BTF_VAR_GLOBAL_ALLOCATED = 1,
>          BTF_VAR_GLOBAL_EXTERN = 2,
> 
> But after looking at the Patch 3/3, I see you reuse these definitions 
> for functions as well. I just find the names confusing on the first look 
> - "BTF_LINKAGE_STATIC".
> 
> Naming aside, what do you think about adding the defines to 
> include/btf.h instead ?

Actually, I forgot these are defined (separately for both VARs and FUNCs)
in the kernel uapi/linux/btf.h. It would probably be best to mirror that
approach and use a separate enum for each, in include/btf.h. WDYT?

> 
>>   /* Mapping of CTF variables to the IDs they will be assigned when they are
>>      converted to BTF_KIND_VAR type records. Strictly accounts for the index
>>      from the start of the variable type entries, does not include the number
>> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>   	continue;
>>   
>>         const char *section_name = node->get_section ();
>> +      /* Mark extern variables.  */
>> +      if (DECL_EXTERNAL (node->decl))
>> +	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>   
> 
> This made me think about the following case.
> 
> extern const char a[];
> const char a[] = "foo";
> 
> What is the expected BTF for this? Since BTF can differentiate between 
> the non-defining extern variable declaration, I expected to see two 
> variables with different "linkage". At this time I see, two variables 
> with global linkage but different types:
> 
>          .long   0xe000000       # btv_info
>          .long   0x4     # btv_type
>          .long   0x1     # btv_linkage
>          .long   0x1f    # btv_name
>          .long   0xe000000       # btv_info
>          .long   0x7     # btv_type
>          .long   0x1     # btv_linkage
>          .long   0x60    # btt_name
> 

The BTF documentation in the kernel does not clarify this case.
Going off the implementation in clang as a reference, it looks like
only one VAR record is expected, with 'global' linkage:

$ cat extdef.c                                
extern const char a[];
const char a[] = "foo";

$ clang -target bpf -c -g extdef.c -o extdef.o

$ /usr/sbin/bpftool btf dump file extdef.o       
[1] CONST '(anon)' type_id=2
[2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
[3] ARRAY '(anon)' type_id=1 index_type_id=4 nr_elems=4
[4] INT '__ARRAY_SIZE_TYPE__' size=4 bits_offset=0 nr_bits=32 encoding=(none)
[5] VAR 'a' type_id=3, linkage=global
[6] DATASEC '.rodata' size=0 vlen=1
	type_id=5 offset=0 size=4 (VAR 'a')

In GCC we have two records since we have two DIEs for "a" in the
DWARF. One has type "const char[4]" and the other has type
"const char[]", so the BTF records point to two different types
as well.

I guess we should find a way in BTF to identify this and
emit only the defining definition as clang does.


>>         if (section_name == NULL)
>>   	{
>> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
>>     dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
>>     dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
>>     dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
>> -  dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
>> +  dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
>>   }
>>   
>>   /* Asm'out a member description following a BTF_KIND_STRUCT or
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>> new file mode 100644
>> index 00000000000..d77600bae1c
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>> @@ -0,0 +1,24 @@
>> +/* Test BTF generation for extern variables.  */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect 4 variables.  */
>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
>> +
>> +/* 2 extern, 1 global, 1 static.  */
>> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
>> +
>> +extern int a;
>> +extern const int b;
>> +int c;
>> +static const int d = 5;
>> +
>> +int foo (int x)
>> +{
>> +  c = a + b + x;
>> +
>> +  return c + d;
>> +}
>> diff --git a/include/btf.h b/include/btf.h
>> index eba67f9d599..9a757ce5bc9 100644
>> --- a/include/btf.h
>> +++ b/include/btf.h
>> @@ -182,7 +182,7 @@ struct btf_param
>>      information about the variable.  */
>>   struct btf_var
>>   {
>> -  uint32_t linkage;	/* Currently only 0=static or 1=global.  */
>> +  uint32_t linkage;	/* 0=static, 1=global, 2=extern.  */
>>   };
>>   
>>   /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
> 

Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]

[Indu Bhagat]

On 12/12/22 12:47, David Faust wrote:
> 
> 
> On 12/8/22 22:55, Indu Bhagat wrote:
>> Hi David,
>>
>> On 12/7/22 12:57, David Faust wrote:
>>> Add support for the 'extern' linkage value for BTF_KIND_VAR records,
>>> which is used for variables declared as extern in the source file.
>>>
>>> 	PR target/106773
>>>
>>> gcc/
>>>
>>> 	* btfout.cc (BTF_LINKAGE_STATIC): New define.
>>> 	(BTF_LINKAGE_GLOBAL): Likewise.
>>> 	(BTF_LINKAGE_EXTERN): Likewise.
>>> 	(btf_collect_datasec): Mark extern variables as such.
>>> 	(btf_asm_varent): Accomodate 'extern' linkage.
>>>
>>> gcc/testsuite/
>>>
>>> 	* gcc.dg/debug/btf/btf-variables-4.c: New test.
>>>
>>> include/
>>>
>>> 	* btf.h (struct btf_var): Update comment to note 'extern' linkage.
>>> ---
>>>    gcc/btfout.cc                                 |  9 ++++++-
>>>    .../gcc.dg/debug/btf/btf-variables-4.c        | 24 +++++++++++++++++++
>>>    include/btf.h                                 |  2 +-
>>>    3 files changed, 33 insertions(+), 2 deletions(-)
>>>    create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index aef9fd70a28..a1c6266a7db 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -66,6 +66,10 @@ static char btf_info_section_label[MAX_BTF_LABEL_BYTES];
>>>    
>>>    #define BTF_INVALID_TYPEID 0xFFFFFFFF
>>>    
>>> +#define BTF_LINKAGE_STATIC 0
>>> +#define BTF_LINKAGE_GLOBAL 1
>>> +#define BTF_LINKAGE_EXTERN 2
>>> +
>>
>> I was about to suggest to rename these to use the same name as used in
>> the kernel btf.h. What is used there is:
>>           BTF_VAR_STATIC = 0,
>>           BTF_VAR_GLOBAL_ALLOCATED = 1,
>>           BTF_VAR_GLOBAL_EXTERN = 2,
>>
>> But after looking at the Patch 3/3, I see you reuse these definitions
>> for functions as well. I just find the names confusing on the first look
>> - "BTF_LINKAGE_STATIC".
>>
>> Naming aside, what do you think about adding the defines to
>> include/btf.h instead ?
> 
> Actually, I forgot these are defined (separately for both VARs and FUNCs)
> in the kernel uapi/linux/btf.h. It would probably be best to mirror that
> approach and use a separate enum for each, in include/btf.h. WDYT?
> 

Yes, mirroring in include/btf.h sounds good.

>>
>>>    /* Mapping of CTF variables to the IDs they will be assigned when they are
>>>       converted to BTF_KIND_VAR type records. Strictly accounts for the index
>>>       from the start of the variable type entries, does not include the number
>>> @@ -314,6 +318,9 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>>    	continue;
>>>    
>>>          const char *section_name = node->get_section ();
>>> +      /* Mark extern variables.  */
>>> +      if (DECL_EXTERNAL (node->decl))
>>> +	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>>    
>>
>> This made me think about the following case.
>>
>> extern const char a[];
>> const char a[] = "foo";
>>
>> What is the expected BTF for this? Since BTF can differentiate between
>> the non-defining extern variable declaration, I expected to see two
>> variables with different "linkage". At this time I see, two variables
>> with global linkage but different types:
>>
>>           .long   0xe000000       # btv_info
>>           .long   0x4     # btv_type
>>           .long   0x1     # btv_linkage
>>           .long   0x1f    # btv_name
>>           .long   0xe000000       # btv_info
>>           .long   0x7     # btv_type
>>           .long   0x1     # btv_linkage
>>           .long   0x60    # btt_name
>>
> 
> The BTF documentation in the kernel does not clarify this case.
> Going off the implementation in clang as a reference, it looks like
> only one VAR record is expected, with 'global' linkage:
> 
> $ cat extdef.c
> extern const char a[];
> const char a[] = "foo";
> 
> $ clang -target bpf -c -g extdef.c -o extdef.o
> 
> $ /usr/sbin/bpftool btf dump file extdef.o
> [1] CONST '(anon)' type_id=2
> [2] INT 'char' size=1 bits_offset=0 nr_bits=8 encoding=SIGNED
> [3] ARRAY '(anon)' type_id=1 index_type_id=4 nr_elems=4
> [4] INT '__ARRAY_SIZE_TYPE__' size=4 bits_offset=0 nr_bits=32 encoding=(none)
> [5] VAR 'a' type_id=3, linkage=global
> [6] DATASEC '.rodata' size=0 vlen=1
> 	type_id=5 offset=0 size=4 (VAR 'a')
> 
> In GCC we have two records since we have two DIEs for "a" in the
> DWARF. One has type "const char[4]" and the other has type
> "const char[]", so the BTF records point to two different types
> as well.
> 
> I guess we should find a way in BTF to identify this and
> emit only the defining definition as clang does.
> 
> 

CTF had a similar issue earlier. See PR105089.
https://gcc.gnu.org/PR105089

>>>          if (section_name == NULL)
>>>    	{
>>> @@ -676,7 +683,7 @@ btf_asm_varent (ctf_dvdef_ref var)
>>>      dw2_asm_output_data (4, var->dvd_name_offset, "btv_name");
>>>      dw2_asm_output_data (4, BTF_TYPE_INFO (BTF_KIND_VAR, 0, 0), "btv_info");
>>>      dw2_asm_output_data (4, get_btf_id (var->dvd_type), "btv_type");
>>> -  dw2_asm_output_data (4, (var->dvd_visibility ? 1 : 0), "btv_linkage");
>>> +  dw2_asm_output_data (4, var->dvd_visibility, "btv_linkage");
>>>    }
>>>    
>>>    /* Asm'out a member description following a BTF_KIND_STRUCT or
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>> new file mode 100644
>>> index 00000000000..d77600bae1c
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-variables-4.c
>>> @@ -0,0 +1,24 @@
>>> +/* Test BTF generation for extern variables.  */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect 4 variables.  */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 4 } } */
>>> +
>>> +/* 2 extern, 1 global, 1 static.  */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x1\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 2 } } */
>>> +
>>> +extern int a;
>>> +extern const int b;
>>> +int c;
>>> +static const int d = 5;
>>> +
>>> +int foo (int x)
>>> +{
>>> +  c = a + b + x;
>>> +
>>> +  return c + d;
>>> +}
>>> diff --git a/include/btf.h b/include/btf.h
>>> index eba67f9d599..9a757ce5bc9 100644
>>> --- a/include/btf.h
>>> +++ b/include/btf.h
>>> @@ -182,7 +182,7 @@ struct btf_param
>>>       information about the variable.  */
>>>    struct btf_var
>>>    {
>>> -  uint32_t linkage;	/* Currently only 0=static or 1=global.  */
>>> +  uint32_t linkage;	/* 0=static, 1=global, 2=extern.  */
>>>    };
>>>    
>>>    /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>>

[PATCH 2/3] btf: fix 'extern const void' variables [PR106773]

[David Faust]

The eBPF loader expects to find BTF_KIND_VAR records for references to
extern const void symbols. We were mistakenly identifing these as
unsupported types, and as a result skipping emitting VAR records for
them.

In addition, the internal DWARF representation from which BTF is
produced does not generate 'const' modifier DIEs for the void type,
which meant in BTF the 'const' qualifier was dropped for 'extern const
void' variables. This patch also adds support for generating a const
void type in BTF to correct emission for these variables.

	PR target/106773

gcc/

	* btfout.cc (btf_collect_datasec): Correct size of void entries.
	(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
	refer to void types.
	(btf_init_postprocess): Create 'const void' type record if needed and
	adjust variables to refer to it as appropriate.

gcc/testsuite/

	* gcc.dg/debug/btf/btf-pr106773.c: New test.
---
 gcc/btfout.cc                                 | 44 +++++++++++++++++--
 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
 2 files changed, 65 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c

diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index a1c6266a7db..05f3a3f9b6e 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
       tree size = DECL_SIZE_UNIT (node->decl);
       if (tree_fits_uhwi_p (size))
 	info.size = tree_to_uhwi (size);
+      else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
+	info.size = 1;
 
       /* Offset is left as 0 at compile time, to be filled in by loaders such
 	 as libbpf.  */
@@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
   ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
 
   /* Do not add variables which refer to unsupported types.  */
-  if (btf_removed_type_p (var->dvd_type))
+  if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
     return 1;
 
   arg_ctfc->ctfc_vars_list[num_vars_added] = var;
@@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
 {
   ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
 
-  size_t i;
-  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
-
   holes.create (0);
   voids.create (0);
 
   num_types_added = 0;
   num_types_created = 0;
 
+  /* Workaround for 'const void' variables. These variables are sometimes used
+     in eBPF programs to address kernel symbols. DWARF does not generate const
+     qualifier on void type, so we would incorrectly emit these variables
+     without the const qualifier.
+     Unfortunately we need the TREE node to know it was const, and we need
+     to create the const modifier type (if needed) now, before making the types
+     list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
+     again when creating the DATASEC entries.  */
+  ctf_id_t constvoid_id = CTF_NULL_TYPEID;
+  varpool_node *var;
+  FOR_EACH_VARIABLE (var)
+    {
+      if (!var->decl)
+	continue;
+
+      tree type = TREE_TYPE (var->decl);
+      if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
+	{
+	  dw_die_ref die = lookup_decl_die (var->decl);
+	  if (die == NULL)
+	    continue;
+
+	  ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
+	  if (dvd == NULL)
+	    continue;
+
+	  /* Create the 'const' modifier type for void.  */
+	  if (constvoid_id == CTF_NULL_TYPEID)
+	    constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
+					    dvd->dvd_type, CTF_K_CONST, NULL);
+	  dvd->dvd_type = constvoid_id;
+	}
+    }
+
+  size_t i;
+  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
+
   if (num_ctf_types)
     {
       init_btf_id_map (num_ctf_types + 1);
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
new file mode 100644
index 00000000000..f90fa773a4b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
@@ -0,0 +1,25 @@
+/* Test BTF generation for extern const void symbols.
+   BTF_KIND_VAR records should be emitted for such symbols if they are used,
+   as well as a corresponding entry in the appropriate DATASEC record.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect 1 variable record only for foo, with 'extern' (2) linkage.  */
+/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
+/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
+
+/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
+
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
+
+extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
+extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
+
+unsigned long func () {
+  unsigned long x = (unsigned long) &foo;
+
+  return x;
+}
+
-- 
2.38.1

Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]

[Indu Bhagat]

Looks OK to me overall. Minor comments below.

Thanks

On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find BTF_KIND_VAR records for references to
> extern const void symbols. We were mistakenly identifing these as
> unsupported types, and as a result skipping emitting VAR records for
> them.
> 
> In addition, the internal DWARF representation from which BTF is
> produced does not generate 'const' modifier DIEs for the void type,
> which meant in BTF the 'const' qualifier was dropped for 'extern const
> void' variables. This patch also adds support for generating a const
> void type in BTF to correct emission for these variables.
> 
> 	PR target/106773
> 
> gcc/
> 
> 	* btfout.cc (btf_collect_datasec): Correct size of void entries.
> 	(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
> 	refer to void types.
> 	(btf_init_postprocess): Create 'const void' type record if needed and
> 	adjust variables to refer to it as appropriate.
> 
> gcc/testsuite/
> 
> 	* gcc.dg/debug/btf/btf-pr106773.c: New test.
> ---
>   gcc/btfout.cc                                 | 44 +++++++++++++++++--
>   gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>   2 files changed, 65 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index a1c6266a7db..05f3a3f9b6e 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>         tree size = DECL_SIZE_UNIT (node->decl);
>         if (tree_fits_uhwi_p (size))
>   	info.size = tree_to_uhwi (size);
> +      else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
> +	info.size = 1;
>   
>         /* Offset is left as 0 at compile time, to be filled in by loaders such
>   	 as libbpf.  */
> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>     ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>   
>     /* Do not add variables which refer to unsupported types.  */
> -  if (btf_removed_type_p (var->dvd_type))
> +  if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>       return 1;
>   
>     arg_ctfc->ctfc_vars_list[num_vars_added] = var;
> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>   {
>     ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>   
> -  size_t i;
> -  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> -
>     holes.create (0);
>     voids.create (0);
>   
>     num_types_added = 0;
>     num_types_created = 0;
>   
> +  /* Workaround for 'const void' variables. These variables are sometimes used
> +     in eBPF programs to address kernel symbols. DWARF does not generate const
> +     qualifier on void type, so we would incorrectly emit these variables
> +     without the const qualifier.
> +     Unfortunately we need the TREE node to know it was const, and we need
> +     to create the const modifier type (if needed) now, before making the types
> +     list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
> +     again when creating the DATASEC entries.  */

"Dot, space, space, new sentence." in 3 places.


> +  ctf_id_t constvoid_id = CTF_NULL_TYPEID;
> +  varpool_node *var;
> +  FOR_EACH_VARIABLE (var)
> +    {
> +      if (!var->decl)
> +	continue;
> +
> +      tree type = TREE_TYPE (var->decl);
> +      if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
> +	{
> +	  dw_die_ref die = lookup_decl_die (var->decl);
> +	  if (die == NULL)
> +	    continue;
> +
> +	  ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
> +	  if (dvd == NULL)
> +	    continue;
> +
> +	  /* Create the 'const' modifier type for void.  */
> +	  if (constvoid_id == CTF_NULL_TYPEID)
> +	    constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
> +					    dvd->dvd_type, CTF_K_CONST, NULL);

No de-duplication of the const void type.  I assume libbpf will take 
care of this eventually.

> +	  dvd->dvd_type = constvoid_id;
> +	}
> +    }
> +
> +  size_t i;
> +  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
> +
>     if (num_ctf_types)
>       {
>         init_btf_id_map (num_ctf_types + 1);
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> new file mode 100644
> index 00000000000..f90fa773a4b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> @@ -0,0 +1,25 @@
> +/* Test BTF generation for extern const void symbols.
> +   BTF_KIND_VAR records should be emitted for such symbols if they are used,
> +   as well as a corresponding entry in the appropriate DATASEC record.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage.  */
> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
> +
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
> +
> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
> +
> +unsigned long func () {
> +  unsigned long x = (unsigned long) &foo;
> +
> +  return x;
> +}
> +

Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]

[David Faust]

On 12/8/22 23:34, Indu Bhagat wrote:
> Looks OK to me overall. Minor comments below.
> 
> Thanks
> 
> On 12/7/22 12:57, David Faust wrote:
>> The eBPF loader expects to find BTF_KIND_VAR records for references to
>> extern const void symbols. We were mistakenly identifing these as
>> unsupported types, and as a result skipping emitting VAR records for
>> them.
>>
>> In addition, the internal DWARF representation from which BTF is
>> produced does not generate 'const' modifier DIEs for the void type,
>> which meant in BTF the 'const' qualifier was dropped for 'extern const
>> void' variables. This patch also adds support for generating a const
>> void type in BTF to correct emission for these variables.
>>
>> 	PR target/106773
>>
>> gcc/
>>
>> 	* btfout.cc (btf_collect_datasec): Correct size of void entries.
>> 	(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
>> 	refer to void types.
>> 	(btf_init_postprocess): Create 'const void' type record if needed and
>> 	adjust variables to refer to it as appropriate.
>>
>> gcc/testsuite/
>>
>> 	* gcc.dg/debug/btf/btf-pr106773.c: New test.
>> ---
>>   gcc/btfout.cc                                 | 44 +++++++++++++++++--
>>   gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>>   2 files changed, 65 insertions(+), 4 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index a1c6266a7db..05f3a3f9b6e 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>         tree size = DECL_SIZE_UNIT (node->decl);
>>         if (tree_fits_uhwi_p (size))
>>   	info.size = tree_to_uhwi (size);
>> +      else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
>> +	info.size = 1;
>>   
>>         /* Offset is left as 0 at compile time, to be filled in by loaders such
>>   	 as libbpf.  */
>> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>>     ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>>   
>>     /* Do not add variables which refer to unsupported types.  */
>> -  if (btf_removed_type_p (var->dvd_type))
>> +  if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>>       return 1;
>>   
>>     arg_ctfc->ctfc_vars_list[num_vars_added] = var;
>> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>>   {
>>     ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>>   
>> -  size_t i;
>> -  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>> -
>>     holes.create (0);
>>     voids.create (0);
>>   
>>     num_types_added = 0;
>>     num_types_created = 0;
>>   
>> +  /* Workaround for 'const void' variables. These variables are sometimes used
>> +     in eBPF programs to address kernel symbols. DWARF does not generate const
>> +     qualifier on void type, so we would incorrectly emit these variables
>> +     without the const qualifier.
>> +     Unfortunately we need the TREE node to know it was const, and we need
>> +     to create the const modifier type (if needed) now, before making the types
>> +     list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
>> +     again when creating the DATASEC entries.  */
> 
> "Dot, space, space, new sentence." in 3 places.
> 
> 
>> +  ctf_id_t constvoid_id = CTF_NULL_TYPEID;
>> +  varpool_node *var;
>> +  FOR_EACH_VARIABLE (var)
>> +    {
>> +      if (!var->decl)
>> +	continue;
>> +
>> +      tree type = TREE_TYPE (var->decl);
>> +      if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
>> +	{
>> +	  dw_die_ref die = lookup_decl_die (var->decl);
>> +	  if (die == NULL)
>> +	    continue;
>> +
>> +	  ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
>> +	  if (dvd == NULL)
>> +	    continue;
>> +
>> +	  /* Create the 'const' modifier type for void.  */
>> +	  if (constvoid_id == CTF_NULL_TYPEID)
>> +	    constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
>> +					    dvd->dvd_type, CTF_K_CONST, NULL);
> 
> No de-duplication of the const void type.  I assume libbpf will take 
> care of this eventually.

Hm, not sure I follow. Where is the duplication? The const void type is
only created once here, for the first such variable which needs it, and
reused for subsequent variables. And it does not already exist in the
CTF which we are translating into BTF.

In any case, yes libbpf can handle duplicated types. Though it would
still be good to minimize that where we can to not bloat the BTF info.

> 
>> +	  dvd->dvd_type = constvoid_id;
>> +	}
>> +    }
>> +
>> +  size_t i;
>> +  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>> +
>>     if (num_ctf_types)
>>       {
>>         init_btf_id_map (num_ctf_types + 1);
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> new file mode 100644
>> index 00000000000..f90fa773a4b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> @@ -0,0 +1,25 @@
>> +/* Test BTF generation for extern const void symbols.
>> +   BTF_KIND_VAR records should be emitted for such symbols if they are used,
>> +   as well as a corresponding entry in the appropriate DATASEC record.  */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage.  */
>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>> +
>> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>> +
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>> +
>> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
>> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
>> +
>> +unsigned long func () {
>> +  unsigned long x = (unsigned long) &foo;
>> +
>> +  return x;
>> +}
>> +
> 

Re: [PATCH 2/3] btf: fix 'extern const void' variables [PR106773]

[Indu Bhagat]

On 12/12/22 12:59, David Faust wrote:
> 
> 
> On 12/8/22 23:34, Indu Bhagat wrote:
>> Looks OK to me overall. Minor comments below.
>>
>> Thanks
>>
>> On 12/7/22 12:57, David Faust wrote:
>>> The eBPF loader expects to find BTF_KIND_VAR records for references to
>>> extern const void symbols. We were mistakenly identifing these as
>>> unsupported types, and as a result skipping emitting VAR records for
>>> them.
>>>
>>> In addition, the internal DWARF representation from which BTF is
>>> produced does not generate 'const' modifier DIEs for the void type,
>>> which meant in BTF the 'const' qualifier was dropped for 'extern const
>>> void' variables. This patch also adds support for generating a const
>>> void type in BTF to correct emission for these variables.
>>>
>>> 	PR target/106773
>>>
>>> gcc/
>>>
>>> 	* btfout.cc (btf_collect_datasec): Correct size of void entries.
>>> 	(btf_dvd_emit_preprocess_cb): Do not skip emitting variables which
>>> 	refer to void types.
>>> 	(btf_init_postprocess): Create 'const void' type record if needed and
>>> 	adjust variables to refer to it as appropriate.
>>>
>>> gcc/testsuite/
>>>
>>> 	* gcc.dg/debug/btf/btf-pr106773.c: New test.
>>> ---
>>>    gcc/btfout.cc                                 | 44 +++++++++++++++++--
>>>    gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c | 25 +++++++++++
>>>    2 files changed, 65 insertions(+), 4 deletions(-)
>>>    create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index a1c6266a7db..05f3a3f9b6e 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -354,6 +354,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>>          tree size = DECL_SIZE_UNIT (node->decl);
>>>          if (tree_fits_uhwi_p (size))
>>>    	info.size = tree_to_uhwi (size);
>>> +      else if (VOID_TYPE_P (TREE_TYPE (node->decl)))
>>> +	info.size = 1;
>>>    
>>>          /* Offset is left as 0 at compile time, to be filled in by loaders such
>>>    	 as libbpf.  */
>>> @@ -439,7 +441,7 @@ btf_dvd_emit_preprocess_cb (ctf_dvdef_ref *slot, ctf_container_ref arg_ctfc)
>>>      ctf_dvdef_ref var = (ctf_dvdef_ref) * slot;
>>>    
>>>      /* Do not add variables which refer to unsupported types.  */
>>> -  if (btf_removed_type_p (var->dvd_type))
>>> +  if (!voids.contains (var->dvd_type) && btf_removed_type_p (var->dvd_type))
>>>        return 1;
>>>    
>>>      arg_ctfc->ctfc_vars_list[num_vars_added] = var;
>>> @@ -1073,15 +1075,49 @@ btf_init_postprocess (void)
>>>    {
>>>      ctf_container_ref tu_ctfc = ctf_get_tu_ctfc ();
>>>    
>>> -  size_t i;
>>> -  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>>> -
>>>      holes.create (0);
>>>      voids.create (0);
>>>    
>>>      num_types_added = 0;
>>>      num_types_created = 0;
>>>    
>>> +  /* Workaround for 'const void' variables. These variables are sometimes used
>>> +     in eBPF programs to address kernel symbols. DWARF does not generate const
>>> +     qualifier on void type, so we would incorrectly emit these variables
>>> +     without the const qualifier.
>>> +     Unfortunately we need the TREE node to know it was const, and we need
>>> +     to create the const modifier type (if needed) now, before making the types
>>> +     list. So we can't avoid iterating with FOR_EACH_VARIABLE here, and then
>>> +     again when creating the DATASEC entries.  */
>>
>> "Dot, space, space, new sentence." in 3 places.
>>
>>
>>> +  ctf_id_t constvoid_id = CTF_NULL_TYPEID;
>>> +  varpool_node *var;
>>> +  FOR_EACH_VARIABLE (var)
>>> +    {
>>> +      if (!var->decl)
>>> +	continue;
>>> +
>>> +      tree type = TREE_TYPE (var->decl);
>>> +      if (type && VOID_TYPE_P (type) && TYPE_READONLY (type))
>>> +	{
>>> +	  dw_die_ref die = lookup_decl_die (var->decl);
>>> +	  if (die == NULL)
>>> +	    continue;
>>> +
>>> +	  ctf_dvdef_ref dvd = ctf_dvd_lookup (tu_ctfc, die);
>>> +	  if (dvd == NULL)
>>> +	    continue;
>>> +
>>> +	  /* Create the 'const' modifier type for void.  */
>>> +	  if (constvoid_id == CTF_NULL_TYPEID)
>>> +	    constvoid_id = ctf_add_reftype (tu_ctfc, CTF_ADD_ROOT,
>>> +					    dvd->dvd_type, CTF_K_CONST, NULL);
>>
>> No de-duplication of the const void type.  I assume libbpf will take
>> care of this eventually.
> 
> Hm, not sure I follow. Where is the duplication? The const void type is
> only created once here, for the first such variable which needs it, and
> reused for subsequent variables. And it does not already exist in the
> CTF which we are translating into BTF.
> 

You're right - you are reusing the const void type once generated for a 
CU for each usage. My bad - I didnt follow the code properly :)

> In any case, yes libbpf can handle duplicated types. Though it would
> still be good to minimize that where we can to not bloat the BTF info.
> 
>>
>>> +	  dvd->dvd_type = constvoid_id;
>>> +	}
>>> +    }
>>> +
>>> +  size_t i;
>>> +  size_t num_ctf_types = tu_ctfc->ctfc_types->elements ();
>>> +
>>>      if (num_ctf_types)
>>>        {
>>>          init_btf_id_map (num_ctf_types + 1);
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>> new file mode 100644
>>> index 00000000000..f90fa773a4b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>>> @@ -0,0 +1,25 @@
>>> +/* Test BTF generation for extern const void symbols.
>>> +   BTF_KIND_VAR records should be emitted for such symbols if they are used,
>>> +   as well as a corresponding entry in the appropriate DATASEC record.  */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect 1 variable record only for foo, with 'extern' (2) linkage.  */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0xe000000\[\t \]+\[^\n\]*btv_info" 1 } } */
>>> +/* { dg-final { scan-assembler-times "\[\t \]0x2\[\t \]+\[^\n\]*btv_linkage" 1 } } */
>>> +
>>> +/* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>>> +
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
>>> +/* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>>> +
>>> +extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
>>> +extern const void bar __attribute__((weak)) __attribute__((section (".ksyms")));
>>> +
>>> +unsigned long func () {
>>> +  unsigned long x = (unsigned long) &foo;
>>> +
>>> +  return x;
>>> +}
>>> +
>>

[PATCH 3/3] btf: correct generation for extern funcs [PR106773]

[David Faust]

The eBPF loader expects to find entries for functions declared as extern
in the corresponding BTF_KIND_DATASEC record, but we were not generating
these entries.

This patch adds support for the 'extern' linkage of function types in
BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.

	PR target/106773

gcc/

	* btfout.cc (get_section_name): New function.
	(btf_collect_datasec): Use it here. Process functions, marking them
	'extern' and generating DATASEC entries for them as appropriate. Move
	creation of BTF_KIND_FUNC records to here...
	(btf_dtd_emit_preprocess_cb): ... from here.

gcc/testsuite/

	* gcc.dg/debug/btf/btf-datasec-2.c: New test.
	* gcc.dg/debug/btf/btf-function-6.c: New test.

include/

	* btf.h (struct btf_var_secinfo): Update comments with notes about
	extern functions.
---
 gcc/btfout.cc                                 | 129 ++++++++++++------
 .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 ++++
 .../gcc.dg/debug/btf/btf-function-6.c         |  19 +++
 include/btf.h                                 |   9 +-
 4 files changed, 139 insertions(+), 46 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
 create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c

diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index 05f3a3f9b6e..d7ead377ec5 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
   ds.entries.safe_push (info);
 
   datasecs.safe_push (ds);
-  num_types_created++;
+}
+
+
+/* Return the section name, as of interest to btf_collect_datasec, for the
+   given symtab node. Note that this deliberately returns NULL for objects
+   which do not go in a section btf_collect_datasec cares about.  */
+static const char *
+get_section_name (symtab_node *node)
+{
+  const char *section_name = node->get_section ();
+
+  if (section_name == NULL)
+    {
+      switch (categorize_decl_for_section (node->decl, 0))
+	{
+	case SECCAT_BSS:
+	  section_name = ".bss";
+	  break;
+	case SECCAT_DATA:
+	  section_name = ".data";
+	  break;
+	case SECCAT_RODATA:
+	  section_name = ".rodata";
+	  break;
+	default:;
+	}
+    }
+
+  return section_name;
 }
 
 /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
@@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
 static void
 btf_collect_datasec (ctf_container_ref ctfc)
 {
-  /* See cgraph.h struct symtab_node, which varpool_node extends.  */
+  cgraph_node *func;
+  FOR_EACH_FUNCTION (func)
+    {
+      dw_die_ref die = lookup_decl_die (func->decl);
+      if (die == NULL)
+	continue;
+
+      ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
+      if (dtd == NULL)
+	continue;
+
+      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
+	 also a BTF_KIND_FUNC. But the CTF container only allocates one
+	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
+	 For each such function, also allocate a BTF_KIND_FUNC entry.
+	 These will be output later.  */
+      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
+      func_dtd->dtd_data = dtd->dtd_data;
+      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
+      func_dtd->linkage = dtd->linkage;
+      func_dtd->dtd_type = num_types_added + num_types_created;
+
+      /* Only the BTF_KIND_FUNC type actually references the name. The
+	 BTF_KIND_FUNC_PROTO is always anonymous.  */
+      dtd->dtd_data.ctti_name = 0;
+
+      vec_safe_push (funcs, func_dtd);
+      num_types_created++;
+
+      /* Mark any 'extern' funcs and add DATASEC entries for them.  */
+      if (DECL_EXTERNAL (func->decl))
+	{
+	  func_dtd->linkage = BTF_LINKAGE_EXTERN;
+
+	  const char *section_name = get_section_name (func);
+	  /* Note: get_section_name () returns NULL for functions in text
+	     section. This is intentional, since we do not want to generate
+	     DATASEC entries for them.  */
+	  if (section_name == NULL)
+	    continue;
+
+	  struct btf_var_secinfo info;
+
+	  /* +1 for the sentinel type not in the types map.  */
+	  info.type = func_dtd->dtd_type + 1;
+
+	  /* Both zero at compile time.  */
+	  info.size = 0;
+	  info.offset = 0;
+
+	  btf_datasec_push_entry (ctfc, section_name, info);
+	}
+    }
+
   varpool_node *node;
   FOR_EACH_VARIABLE (node)
     {
@@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
       if (dvd == NULL)
 	continue;
 
-      const char *section_name = node->get_section ();
       /* Mark extern variables.  */
       if (DECL_EXTERNAL (node->decl))
 	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
 
+      const char *section_name = get_section_name (node);
       if (section_name == NULL)
-	{
-	  switch (categorize_decl_for_section (node->decl, 0))
-	    {
-	    case SECCAT_BSS:
-	      section_name = ".bss";
-	      break;
-	    case SECCAT_DATA:
-	      section_name = ".data";
-	      break;
-	    case SECCAT_RODATA:
-	      section_name = ".rodata";
-	      break;
-	    default:
-	      continue;
-	    }
-	}
+	continue;
 
       struct btf_var_secinfo info;
 
@@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
 
       btf_datasec_push_entry (ctfc, section_name, info);
     }
+
+  num_types_created += datasecs.length ();
 }
 
 /* Return true if the type ID is that of a type which will not be emitted (for
@@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
   if (!btf_emit_id_p (dtd->dtd_type))
     return;
 
-  uint32_t btf_kind
-    = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
-
-  if (btf_kind == BTF_KIND_FUNC_PROTO)
-    {
-      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
-	 also a BTF_KIND_FUNC. But the CTF container only allocates one
-	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
-	 For each such function, also allocate a BTF_KIND_FUNC entry.
-	 These will be output later.  */
-      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
-      func_dtd->dtd_data = dtd->dtd_data;
-      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
-      func_dtd->linkage = dtd->linkage;
-
-      vec_safe_push (funcs, func_dtd);
-      num_types_created++;
-
-      /* Only the BTF_KIND_FUNC type actually references the name. The
-	 BTF_KIND_FUNC_PROTO is always anonymous.  */
-      dtd->dtd_data.ctti_name = 0;
-    }
-
   ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
 }
 
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
new file mode 100644
index 00000000000..f4b298cf019
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
@@ -0,0 +1,28 @@
+/* Test BTF generation of DATASEC records for extern functions.
+
+   Only functions declared extern should have entries in DATASEC records.  */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
+/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
+/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
+
+/* Function entries should have offset and size of 0 at compile time.  */
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
+/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
+
+extern int foo (int a) __attribute__((section(".foo_sec")));
+
+
+extern int bar (int b) __attribute__((section(".bar_sec")));
+extern void chacha (void) __attribute__((section(".bar_sec")));
+
+__attribute__((section(".foo_sec")))
+void baz (int *x)
+{
+  chacha ();
+
+  *x = foo (bar (*x));
+}
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
new file mode 100644
index 00000000000..48a946ab14b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
@@ -0,0 +1,19 @@
+/* Test BTF extern linkage for functions.
+
+   We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
+   one BTF_KIND_FUNC type with extern linkage (extfunc).  */
+
+/* { dg-do compile } */
+/* { dg-options "-O0 -gbtf -dA" } */
+
+/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
+/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
+
+extern int extfunc(int a, int b);
+
+int foo (int x) {
+
+  int y = extfunc (x, x+1);
+
+  return y;
+}
diff --git a/include/btf.h b/include/btf.h
index 9a757ce5bc9..f41ea94b75f 100644
--- a/include/btf.h
+++ b/include/btf.h
@@ -186,12 +186,13 @@ struct btf_var
 };
 
 /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
-   which describe all BTF_KIND_VAR types contained in the section.  */
+   which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
+   in the section.  */
 struct btf_var_secinfo
 {
-  uint32_t type;	/* Type of variable.  */
-  uint32_t offset;	/* In-section offset of variable (in bytes).  */
-  uint32_t size;	/* Size (in bytes) of variable.  */
+  uint32_t type;	/* Type of BTF_KIND_VAR or BTF_KIND_FUNC item.  */
+  uint32_t offset;	/* In-section offset (in bytes) of item.  */
+  uint32_t size;	/* Size (in bytes) of item.  */
 };
 
 /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
-- 
2.38.1

Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]

[Indu Bhagat]

On 12/7/22 12:57, David Faust wrote:
> The eBPF loader expects to find entries for functions declared as extern
> in the corresponding BTF_KIND_DATASEC record, but we were not generating
> these entries.
> 
> This patch adds support for the 'extern' linkage of function types in
> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
> 
> 	PR target/106773
> 
> gcc/
> 
> 	* btfout.cc (get_section_name): New function.
> 	(btf_collect_datasec): Use it here. Process functions, marking them
> 	'extern' and generating DATASEC entries for them as appropriate. Move
> 	creation of BTF_KIND_FUNC records to here...
> 	(btf_dtd_emit_preprocess_cb): ... from here.
> 
> gcc/testsuite/
> 
> 	* gcc.dg/debug/btf/btf-datasec-2.c: New test.
> 	* gcc.dg/debug/btf/btf-function-6.c: New test.
> 
> include/
> 
> 	* btf.h (struct btf_var_secinfo): Update comments with notes about
> 	extern functions.
> ---
>   gcc/btfout.cc                                 | 129 ++++++++++++------
>   .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 ++++
>   .../gcc.dg/debug/btf/btf-function-6.c         |  19 +++
>   include/btf.h                                 |   9 +-
>   4 files changed, 139 insertions(+), 46 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index 05f3a3f9b6e..d7ead377ec5 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>     ds.entries.safe_push (info);
>   
>     datasecs.safe_push (ds);
> -  num_types_created++;
> +}
> +
> +
> +/* Return the section name, as of interest to btf_collect_datasec, for the
> +   given symtab node. Note that this deliberately returns NULL for objects
> +   which do not go in a section btf_collect_datasec cares about.  */

"Dot, space, space, new sentence."

> +static const char *
> +get_section_name (symtab_node *node)
> +{
> +  const char *section_name = node->get_section ();
> +
> +  if (section_name == NULL)
> +    {
> +      switch (categorize_decl_for_section (node->decl, 0))
> +	{
> +	case SECCAT_BSS:
> +	  section_name = ".bss";
> +	  break;
> +	case SECCAT_DATA:
> +	  section_name = ".data";
> +	  break;
> +	case SECCAT_RODATA:
> +	  section_name = ".rodata";
> +	  break;
> +	default:;
> +	}
> +    }
> +
> +  return section_name;
>   }
>   
>   /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>   static void
>   btf_collect_datasec (ctf_container_ref ctfc)
>   {
> -  /* See cgraph.h struct symtab_node, which varpool_node extends.  */
> +  cgraph_node *func;
> +  FOR_EACH_FUNCTION (func)
> +    {
> +      dw_die_ref die = lookup_decl_die (func->decl);
> +      if (die == NULL)
> +	continue;
> +
> +      ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
> +      if (dtd == NULL)
> +	continue;
> +
> +      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> +	 also a BTF_KIND_FUNC. But the CTF container only allocates one
> +	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> +	 For each such function, also allocate a BTF_KIND_FUNC entry.
> +	 These will be output later.  */

"Dot, space, space, new sentence."

> +      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> +      func_dtd->dtd_data = dtd->dtd_data;
> +      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> +      func_dtd->linkage = dtd->linkage;
> +      func_dtd->dtd_type = num_types_added + num_types_created;
> +
> +      /* Only the BTF_KIND_FUNC type actually references the name. The
> +	 BTF_KIND_FUNC_PROTO is always anonymous.  */
> +      dtd->dtd_data.ctti_name = 0;
> +
> +      vec_safe_push (funcs, func_dtd);
> +      num_types_created++;
> +
> +      /* Mark any 'extern' funcs and add DATASEC entries for them.  */
> +      if (DECL_EXTERNAL (func->decl))
> +	{
> +	  func_dtd->linkage = BTF_LINKAGE_EXTERN;
> +

What is the expected BTF when both decl and definition are present:

extern int extfunc(int x);
int extfunc (int x) {
   int y = foo ();
   return y;
}

> +	  const char *section_name = get_section_name (func);
> +	  /* Note: get_section_name () returns NULL for functions in text
> +	     section. This is intentional, since we do not want to generate
> +	     DATASEC entries for them.  */

"Dot, space, space, new sentence."

> +	  if (section_name == NULL)
> +	    continue;
> +
> +	  struct btf_var_secinfo info;
> +
> +	  /* +1 for the sentinel type not in the types map.  */
> +	  info.type = func_dtd->dtd_type + 1;
> +
> +	  /* Both zero at compile time.  */
> +	  info.size = 0;
> +	  info.offset = 0;
> +
> +	  btf_datasec_push_entry (ctfc, section_name, info);
> +	}
> +    }
> +
>     varpool_node *node;
>     FOR_EACH_VARIABLE (node)
>       {
> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>         if (dvd == NULL)
>   	continue;
>   
> -      const char *section_name = node->get_section ();
>         /* Mark extern variables.  */
>         if (DECL_EXTERNAL (node->decl))
>   	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>   
> +      const char *section_name = get_section_name (node);
>         if (section_name == NULL)
> -	{
> -	  switch (categorize_decl_for_section (node->decl, 0))
> -	    {
> -	    case SECCAT_BSS:
> -	      section_name = ".bss";
> -	      break;
> -	    case SECCAT_DATA:
> -	      section_name = ".data";
> -	      break;
> -	    case SECCAT_RODATA:
> -	      section_name = ".rodata";
> -	      break;
> -	    default:
> -	      continue;
> -	    }
> -	}
> +	continue;
>   
>         struct btf_var_secinfo info;
>   
> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>   
>         btf_datasec_push_entry (ctfc, section_name, info);
>       }
> +
> +  num_types_created += datasecs.length ();
>   }
>   
>   /* Return true if the type ID is that of a type which will not be emitted (for
> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>     if (!btf_emit_id_p (dtd->dtd_type))
>       return;
>   
> -  uint32_t btf_kind
> -    = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
> -
> -  if (btf_kind == BTF_KIND_FUNC_PROTO)
> -    {
> -      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
> -	 also a BTF_KIND_FUNC. But the CTF container only allocates one
> -	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
> -	 For each such function, also allocate a BTF_KIND_FUNC entry.
> -	 These will be output later.  */
> -      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
> -      func_dtd->dtd_data = dtd->dtd_data;
> -      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
> -      func_dtd->linkage = dtd->linkage;
> -
> -      vec_safe_push (funcs, func_dtd);
> -      num_types_created++;
> -
> -      /* Only the BTF_KIND_FUNC type actually references the name. The
> -	 BTF_KIND_FUNC_PROTO is always anonymous.  */
> -      dtd->dtd_data.ctti_name = 0;
> -    }
> -
>     ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>   }
>   
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> new file mode 100644
> index 00000000000..f4b298cf019
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> @@ -0,0 +1,28 @@
> +/* Test BTF generation of DATASEC records for extern functions.
> +
> +   Only functions declared extern should have entries in DATASEC records.  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
> +
> +/* Function entries should have offset and size of 0 at compile time.  */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
> +
> +extern int foo (int a) __attribute__((section(".foo_sec")));
> +
> +
> +extern int bar (int b) __attribute__((section(".bar_sec")));
> +extern void chacha (void) __attribute__((section(".bar_sec")));
> +
> +__attribute__((section(".foo_sec")))
> +void baz (int *x)
> +{
> +  chacha ();
> +
> +  *x = foo (bar (*x));
> +}
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> new file mode 100644
> index 00000000000..48a946ab14b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
> @@ -0,0 +1,19 @@
> +/* Test BTF extern linkage for functions.
> +
> +   We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
> +   one BTF_KIND_FUNC type with extern linkage (extfunc).  */
> +
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -gbtf -dA" } */
> +
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
> +
> +extern int extfunc(int a, int b);
> +
> +int foo (int x) {
> +
> +  int y = extfunc (x, x+1);
> +
> +  return y;
> +}
> diff --git a/include/btf.h b/include/btf.h
> index 9a757ce5bc9..f41ea94b75f 100644
> --- a/include/btf.h
> +++ b/include/btf.h
> @@ -186,12 +186,13 @@ struct btf_var
>   };
>   
>   /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
> -   which describe all BTF_KIND_VAR types contained in the section.  */
> +   which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
> +   in the section.  */
>   struct btf_var_secinfo
>   {
> -  uint32_t type;	/* Type of variable.  */
> -  uint32_t offset;	/* In-section offset of variable (in bytes).  */
> -  uint32_t size;	/* Size (in bytes) of variable.  */
> +  uint32_t type;	/* Type of BTF_KIND_VAR or BTF_KIND_FUNC item.  */
> +  uint32_t offset;	/* In-section offset (in bytes) of item.  */
> +  uint32_t size;	/* Size (in bytes) of item.  */
>   };
>   
>   /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,

Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]

[David Faust]

On 12/8/22 23:36, Indu Bhagat wrote:
> On 12/7/22 12:57, David Faust wrote:
>> The eBPF loader expects to find entries for functions declared as extern
>> in the corresponding BTF_KIND_DATASEC record, but we were not generating
>> these entries.
>>
>> This patch adds support for the 'extern' linkage of function types in
>> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
>>
>> 	PR target/106773
>>
>> gcc/
>>
>> 	* btfout.cc (get_section_name): New function.
>> 	(btf_collect_datasec): Use it here. Process functions, marking them
>> 	'extern' and generating DATASEC entries for them as appropriate. Move
>> 	creation of BTF_KIND_FUNC records to here...
>> 	(btf_dtd_emit_preprocess_cb): ... from here.
>>
>> gcc/testsuite/
>>
>> 	* gcc.dg/debug/btf/btf-datasec-2.c: New test.
>> 	* gcc.dg/debug/btf/btf-function-6.c: New test.
>>
>> include/
>>
>> 	* btf.h (struct btf_var_secinfo): Update comments with notes about
>> 	extern functions.
>> ---
>>   gcc/btfout.cc                                 | 129 ++++++++++++------
>>   .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 ++++
>>   .../gcc.dg/debug/btf/btf-function-6.c         |  19 +++
>>   include/btf.h                                 |   9 +-
>>   4 files changed, 139 insertions(+), 46 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>   create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index 05f3a3f9b6e..d7ead377ec5 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>     ds.entries.safe_push (info);
>>   
>>     datasecs.safe_push (ds);
>> -  num_types_created++;
>> +}
>> +
>> +
>> +/* Return the section name, as of interest to btf_collect_datasec, for the
>> +   given symtab node. Note that this deliberately returns NULL for objects
>> +   which do not go in a section btf_collect_datasec cares about.  */
> 
> "Dot, space, space, new sentence."
> 
>> +static const char *
>> +get_section_name (symtab_node *node)
>> +{
>> +  const char *section_name = node->get_section ();
>> +
>> +  if (section_name == NULL)
>> +    {
>> +      switch (categorize_decl_for_section (node->decl, 0))
>> +	{
>> +	case SECCAT_BSS:
>> +	  section_name = ".bss";
>> +	  break;
>> +	case SECCAT_DATA:
>> +	  section_name = ".data";
>> +	  break;
>> +	case SECCAT_RODATA:
>> +	  section_name = ".rodata";
>> +	  break;
>> +	default:;
>> +	}
>> +    }
>> +
>> +  return section_name;
>>   }
>>   
>>   /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
>> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>   static void
>>   btf_collect_datasec (ctf_container_ref ctfc)
>>   {
>> -  /* See cgraph.h struct symtab_node, which varpool_node extends.  */
>> +  cgraph_node *func;
>> +  FOR_EACH_FUNCTION (func)
>> +    {
>> +      dw_die_ref die = lookup_decl_die (func->decl);
>> +      if (die == NULL)
>> +	continue;
>> +
>> +      ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
>> +      if (dtd == NULL)
>> +	continue;
>> +
>> +      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>> +	 also a BTF_KIND_FUNC. But the CTF container only allocates one
>> +	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>> +	 For each such function, also allocate a BTF_KIND_FUNC entry.
>> +	 These will be output later.  */
> 
> "Dot, space, space, new sentence."
> 
>> +      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>> +      func_dtd->dtd_data = dtd->dtd_data;
>> +      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>> +      func_dtd->linkage = dtd->linkage;
>> +      func_dtd->dtd_type = num_types_added + num_types_created;
>> +
>> +      /* Only the BTF_KIND_FUNC type actually references the name. The
>> +	 BTF_KIND_FUNC_PROTO is always anonymous.  */
>> +      dtd->dtd_data.ctti_name = 0;
>> +
>> +      vec_safe_push (funcs, func_dtd);
>> +      num_types_created++;
>> +
>> +      /* Mark any 'extern' funcs and add DATASEC entries for them.  */
>> +      if (DECL_EXTERNAL (func->decl))
>> +	{
>> +	  func_dtd->linkage = BTF_LINKAGE_EXTERN;
>> +
> 
> What is the expected BTF when both decl and definition are present:
> 
> extern int extfunc(int x);
> int extfunc (int x) {
>    int y = foo ();
>    return y;
> }

Using clang implementation as the reference, a single FUNC record
for "extfunc" with "global" linkage:

$ cat extfuncdef.c 
extern int extfunc (int x);
int extfunc (int x) {
  int y = foo ();
  return y;
}

$ clang -target bpf -c -g extfuncdef.c -o extfuncdef.o

$ /usr/sbin/bpftool btf dump file extfuncdef.o        
[1] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
	'(anon)' type_id=2
[2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
[3] FUNC 'extfunc' type_id=1 linkage=global

With this patch we do the same in GCC.

> 
>> +	  const char *section_name = get_section_name (func);
>> +	  /* Note: get_section_name () returns NULL for functions in text
>> +	     section. This is intentional, since we do not want to generate
>> +	     DATASEC entries for them.  */
> 
> "Dot, space, space, new sentence."
> 
>> +	  if (section_name == NULL)
>> +	    continue;
>> +
>> +	  struct btf_var_secinfo info;
>> +
>> +	  /* +1 for the sentinel type not in the types map.  */
>> +	  info.type = func_dtd->dtd_type + 1;
>> +
>> +	  /* Both zero at compile time.  */
>> +	  info.size = 0;
>> +	  info.offset = 0;
>> +
>> +	  btf_datasec_push_entry (ctfc, section_name, info);
>> +	}
>> +    }
>> +
>>     varpool_node *node;
>>     FOR_EACH_VARIABLE (node)
>>       {
>> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>         if (dvd == NULL)
>>   	continue;
>>   
>> -      const char *section_name = node->get_section ();
>>         /* Mark extern variables.  */
>>         if (DECL_EXTERNAL (node->decl))
>>   	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>   
>> +      const char *section_name = get_section_name (node);
>>         if (section_name == NULL)
>> -	{
>> -	  switch (categorize_decl_for_section (node->decl, 0))
>> -	    {
>> -	    case SECCAT_BSS:
>> -	      section_name = ".bss";
>> -	      break;
>> -	    case SECCAT_DATA:
>> -	      section_name = ".data";
>> -	      break;
>> -	    case SECCAT_RODATA:
>> -	      section_name = ".rodata";
>> -	      break;
>> -	    default:
>> -	      continue;
>> -	    }
>> -	}
>> +	continue;
>>   
>>         struct btf_var_secinfo info;
>>   
>> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>   
>>         btf_datasec_push_entry (ctfc, section_name, info);
>>       }
>> +
>> +  num_types_created += datasecs.length ();
>>   }
>>   
>>   /* Return true if the type ID is that of a type which will not be emitted (for
>> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>>     if (!btf_emit_id_p (dtd->dtd_type))
>>       return;
>>   
>> -  uint32_t btf_kind
>> -    = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
>> -
>> -  if (btf_kind == BTF_KIND_FUNC_PROTO)
>> -    {
>> -      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>> -	 also a BTF_KIND_FUNC. But the CTF container only allocates one
>> -	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>> -	 For each such function, also allocate a BTF_KIND_FUNC entry.
>> -	 These will be output later.  */
>> -      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>> -      func_dtd->dtd_data = dtd->dtd_data;
>> -      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>> -      func_dtd->linkage = dtd->linkage;
>> -
>> -      vec_safe_push (funcs, func_dtd);
>> -      num_types_created++;
>> -
>> -      /* Only the BTF_KIND_FUNC type actually references the name. The
>> -	 BTF_KIND_FUNC_PROTO is always anonymous.  */
>> -      dtd->dtd_data.ctti_name = 0;
>> -    }
>> -
>>     ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>>   }
>>   
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> new file mode 100644
>> index 00000000000..f4b298cf019
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> @@ -0,0 +1,28 @@
>> +/* Test BTF generation of DATASEC records for extern functions.
>> +
>> +   Only functions declared extern should have entries in DATASEC records.  */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
>> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>> +
>> +/* Function entries should have offset and size of 0 at compile time.  */
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>> +
>> +extern int foo (int a) __attribute__((section(".foo_sec")));
>> +
>> +
>> +extern int bar (int b) __attribute__((section(".bar_sec")));
>> +extern void chacha (void) __attribute__((section(".bar_sec")));
>> +
>> +__attribute__((section(".foo_sec")))
>> +void baz (int *x)
>> +{
>> +  chacha ();
>> +
>> +  *x = foo (bar (*x));
>> +}
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>> new file mode 100644
>> index 00000000000..48a946ab14b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>> @@ -0,0 +1,19 @@
>> +/* Test BTF extern linkage for functions.
>> +
>> +   We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
>> +   one BTF_KIND_FUNC type with extern linkage (extfunc).  */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
>> +
>> +extern int extfunc(int a, int b);
>> +
>> +int foo (int x) {
>> +
>> +  int y = extfunc (x, x+1);
>> +
>> +  return y;
>> +}
>> diff --git a/include/btf.h b/include/btf.h
>> index 9a757ce5bc9..f41ea94b75f 100644
>> --- a/include/btf.h
>> +++ b/include/btf.h
>> @@ -186,12 +186,13 @@ struct btf_var
>>   };
>>   
>>   /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>> -   which describe all BTF_KIND_VAR types contained in the section.  */
>> +   which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
>> +   in the section.  */
>>   struct btf_var_secinfo
>>   {
>> -  uint32_t type;	/* Type of variable.  */
>> -  uint32_t offset;	/* In-section offset of variable (in bytes).  */
>> -  uint32_t size;	/* Size (in bytes) of variable.  */
>> +  uint32_t type;	/* Type of BTF_KIND_VAR or BTF_KIND_FUNC item.  */
>> +  uint32_t offset;	/* In-section offset (in bytes) of item.  */
>> +  uint32_t size;	/* Size (in bytes) of item.  */
>>   };
>>   
>>   /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
> 

Re: [PATCH 3/3] btf: correct generation for extern funcs [PR106773]

[Indu Bhagat]

On 12/12/22 12:31, David Faust wrote:
> 
> 
> On 12/8/22 23:36, Indu Bhagat wrote:
>> On 12/7/22 12:57, David Faust wrote:
>>> The eBPF loader expects to find entries for functions declared as extern
>>> in the corresponding BTF_KIND_DATASEC record, but we were not generating
>>> these entries.
>>>
>>> This patch adds support for the 'extern' linkage of function types in
>>> BTF, and creates entries for for them BTF_KIND_DATASEC records as needed.
>>>
>>> 	PR target/106773
>>>
>>> gcc/
>>>
>>> 	* btfout.cc (get_section_name): New function.
>>> 	(btf_collect_datasec): Use it here. Process functions, marking them
>>> 	'extern' and generating DATASEC entries for them as appropriate. Move
>>> 	creation of BTF_KIND_FUNC records to here...
>>> 	(btf_dtd_emit_preprocess_cb): ... from here.
>>>
>>> gcc/testsuite/
>>>
>>> 	* gcc.dg/debug/btf/btf-datasec-2.c: New test.
>>> 	* gcc.dg/debug/btf/btf-function-6.c: New test.
>>>
>>> include/
>>>
>>> 	* btf.h (struct btf_var_secinfo): Update comments with notes about
>>> 	extern functions.
>>> ---
>>>    gcc/btfout.cc                                 | 129 ++++++++++++------
>>>    .../gcc.dg/debug/btf/btf-datasec-2.c          |  28 ++++
>>>    .../gcc.dg/debug/btf/btf-function-6.c         |  19 +++
>>>    include/btf.h                                 |   9 +-
>>>    4 files changed, 139 insertions(+), 46 deletions(-)
>>>    create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>>    create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>>
>>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>>> index 05f3a3f9b6e..d7ead377ec5 100644
>>> --- a/gcc/btfout.cc
>>> +++ b/gcc/btfout.cc
>>> @@ -294,7 +294,35 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>>      ds.entries.safe_push (info);
>>>    
>>>      datasecs.safe_push (ds);
>>> -  num_types_created++;
>>> +}
>>> +
>>> +
>>> +/* Return the section name, as of interest to btf_collect_datasec, for the
>>> +   given symtab node. Note that this deliberately returns NULL for objects
>>> +   which do not go in a section btf_collect_datasec cares about.  */
>>
>> "Dot, space, space, new sentence."
>>
>>> +static const char *
>>> +get_section_name (symtab_node *node)
>>> +{
>>> +  const char *section_name = node->get_section ();
>>> +
>>> +  if (section_name == NULL)
>>> +    {
>>> +      switch (categorize_decl_for_section (node->decl, 0))
>>> +	{
>>> +	case SECCAT_BSS:
>>> +	  section_name = ".bss";
>>> +	  break;
>>> +	case SECCAT_DATA:
>>> +	  section_name = ".data";
>>> +	  break;
>>> +	case SECCAT_RODATA:
>>> +	  section_name = ".rodata";
>>> +	  break;
>>> +	default:;
>>> +	}
>>> +    }
>>> +
>>> +  return section_name;
>>>    }
>>>    
>>>    /* Construct all BTF_KIND_DATASEC records for CTFC. One such record is created
>>> @@ -305,7 +333,60 @@ btf_datasec_push_entry (ctf_container_ref ctfc, const char *secname,
>>>    static void
>>>    btf_collect_datasec (ctf_container_ref ctfc)
>>>    {
>>> -  /* See cgraph.h struct symtab_node, which varpool_node extends.  */
>>> +  cgraph_node *func;
>>> +  FOR_EACH_FUNCTION (func)
>>> +    {
>>> +      dw_die_ref die = lookup_decl_die (func->decl);
>>> +      if (die == NULL)
>>> +	continue;
>>> +
>>> +      ctf_dtdef_ref dtd = ctf_dtd_lookup (ctfc, die);
>>> +      if (dtd == NULL)
>>> +	continue;
>>> +
>>> +      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>>> +	 also a BTF_KIND_FUNC. But the CTF container only allocates one
>>> +	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>>> +	 For each such function, also allocate a BTF_KIND_FUNC entry.
>>> +	 These will be output later.  */
>>
>> "Dot, space, space, new sentence."
>>
>>> +      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>>> +      func_dtd->dtd_data = dtd->dtd_data;
>>> +      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>>> +      func_dtd->linkage = dtd->linkage;
>>> +      func_dtd->dtd_type = num_types_added + num_types_created;
>>> +
>>> +      /* Only the BTF_KIND_FUNC type actually references the name. The
>>> +	 BTF_KIND_FUNC_PROTO is always anonymous.  */
>>> +      dtd->dtd_data.ctti_name = 0;
>>> +
>>> +      vec_safe_push (funcs, func_dtd);
>>> +      num_types_created++;
>>> +
>>> +      /* Mark any 'extern' funcs and add DATASEC entries for them.  */
>>> +      if (DECL_EXTERNAL (func->decl))
>>> +	{
>>> +	  func_dtd->linkage = BTF_LINKAGE_EXTERN;
>>> +
>>
>> What is the expected BTF when both decl and definition are present:
>>
>> extern int extfunc(int x);
>> int extfunc (int x) {
>>     int y = foo ();
>>     return y;
>> }
> 
> Using clang implementation as the reference, a single FUNC record
> for "extfunc" with "global" linkage:
> 
> $ cat extfuncdef.c
> extern int extfunc (int x);
> int extfunc (int x) {
>    int y = foo ();
>    return y;
> }
> 
> $ clang -target bpf -c -g extfuncdef.c -o extfuncdef.o
> 
> $ /usr/sbin/bpftool btf dump file extfuncdef.o
> [1] FUNC_PROTO '(anon)' ret_type_id=2 vlen=1
> 	'(anon)' type_id=2
> [2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
> [3] FUNC 'extfunc' type_id=1 linkage=global
> 
> With this patch we do the same in GCC.
> 

OK. Thanks for confirming.

>>
>>> +	  const char *section_name = get_section_name (func);
>>> +	  /* Note: get_section_name () returns NULL for functions in text
>>> +	     section. This is intentional, since we do not want to generate
>>> +	     DATASEC entries for them.  */
>>
>> "Dot, space, space, new sentence."
>>
>>> +	  if (section_name == NULL)
>>> +	    continue;
>>> +
>>> +	  struct btf_var_secinfo info;
>>> +
>>> +	  /* +1 for the sentinel type not in the types map.  */
>>> +	  info.type = func_dtd->dtd_type + 1;
>>> +
>>> +	  /* Both zero at compile time.  */
>>> +	  info.size = 0;
>>> +	  info.offset = 0;
>>> +
>>> +	  btf_datasec_push_entry (ctfc, section_name, info);
>>> +	}
>>> +    }
>>> +
>>>      varpool_node *node;
>>>      FOR_EACH_VARIABLE (node)
>>>        {
>>> @@ -317,28 +398,13 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>>          if (dvd == NULL)
>>>    	continue;
>>>    
>>> -      const char *section_name = node->get_section ();
>>>          /* Mark extern variables.  */
>>>          if (DECL_EXTERNAL (node->decl))
>>>    	dvd->dvd_visibility = BTF_LINKAGE_EXTERN;
>>>    
>>> +      const char *section_name = get_section_name (node);
>>>          if (section_name == NULL)
>>> -	{
>>> -	  switch (categorize_decl_for_section (node->decl, 0))
>>> -	    {
>>> -	    case SECCAT_BSS:
>>> -	      section_name = ".bss";
>>> -	      break;
>>> -	    case SECCAT_DATA:
>>> -	      section_name = ".data";
>>> -	      break;
>>> -	    case SECCAT_RODATA:
>>> -	      section_name = ".rodata";
>>> -	      break;
>>> -	    default:
>>> -	      continue;
>>> -	    }
>>> -	}
>>> +	continue;
>>>    
>>>          struct btf_var_secinfo info;
>>>    
>>> @@ -363,6 +429,8 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>>    
>>>          btf_datasec_push_entry (ctfc, section_name, info);
>>>        }
>>> +
>>> +  num_types_created += datasecs.length ();
>>>    }
>>>    
>>>    /* Return true if the type ID is that of a type which will not be emitted (for
>>> @@ -461,29 +529,6 @@ btf_dtd_emit_preprocess_cb (ctf_container_ref ctfc, ctf_dtdef_ref dtd)
>>>      if (!btf_emit_id_p (dtd->dtd_type))
>>>        return;
>>>    
>>> -  uint32_t btf_kind
>>> -    = get_btf_kind (CTF_V2_INFO_KIND (dtd->dtd_data.ctti_info));
>>> -
>>> -  if (btf_kind == BTF_KIND_FUNC_PROTO)
>>> -    {
>>> -      /* Functions actually get two types: a BTF_KIND_FUNC_PROTO, and
>>> -	 also a BTF_KIND_FUNC. But the CTF container only allocates one
>>> -	 type per function, which matches closely with BTF_KIND_FUNC_PROTO.
>>> -	 For each such function, also allocate a BTF_KIND_FUNC entry.
>>> -	 These will be output later.  */
>>> -      ctf_dtdef_ref func_dtd = ggc_cleared_alloc<ctf_dtdef_t> ();
>>> -      func_dtd->dtd_data = dtd->dtd_data;
>>> -      func_dtd->dtd_data.ctti_type = dtd->dtd_type;
>>> -      func_dtd->linkage = dtd->linkage;
>>> -
>>> -      vec_safe_push (funcs, func_dtd);
>>> -      num_types_created++;
>>> -
>>> -      /* Only the BTF_KIND_FUNC type actually references the name. The
>>> -	 BTF_KIND_FUNC_PROTO is always anonymous.  */
>>> -      dtd->dtd_data.ctti_name = 0;
>>> -    }
>>> -
>>>      ctfc->ctfc_num_vlen_bytes += btf_calc_num_vbytes (dtd);
>>>    }
>>>    
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>> new file mode 100644
>>> index 00000000000..f4b298cf019
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>>> @@ -0,0 +1,28 @@
>>> +/* Test BTF generation of DATASEC records for extern functions.
>>> +
>>> +   Only functions declared extern should have entries in DATASEC records.  */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* Expect one DATASEC with vlen=1 (.foo_sec) and one with vlen=2 (.bar_sec) */
>>> +/* { dg-final { scan-assembler-times "0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>>> +/* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>>> +
>>> +/* Function entries should have offset and size of 0 at compile time.  */
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
>>> +/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>>> +
>>> +extern int foo (int a) __attribute__((section(".foo_sec")));
>>> +
>>> +
>>> +extern int bar (int b) __attribute__((section(".bar_sec")));
>>> +extern void chacha (void) __attribute__((section(".bar_sec")));
>>> +
>>> +__attribute__((section(".foo_sec")))
>>> +void baz (int *x)
>>> +{
>>> +  chacha ();
>>> +
>>> +  *x = foo (bar (*x));
>>> +}
>>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>> new file mode 100644
>>> index 00000000000..48a946ab14b
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-function-6.c
>>> @@ -0,0 +1,19 @@
>>> +/* Test BTF extern linkage for functions.
>>> +
>>> +   We expect to see one BTF_KIND_FUNC type with global linkage (foo), and
>>> +   one BTF_KIND_FUNC type with extern linkage (extfunc).  */
>>> +
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O0 -gbtf -dA" } */
>>> +
>>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=2" 1 } } */
>>> +/* { dg-final { scan-assembler-times "btt_info: kind=12, kflag=0, linkage=1" 1 } } */
>>> +
>>> +extern int extfunc(int a, int b);
>>> +
>>> +int foo (int x) {
>>> +
>>> +  int y = extfunc (x, x+1);
>>> +
>>> +  return y;
>>> +}
>>> diff --git a/include/btf.h b/include/btf.h
>>> index 9a757ce5bc9..f41ea94b75f 100644
>>> --- a/include/btf.h
>>> +++ b/include/btf.h
>>> @@ -186,12 +186,13 @@ struct btf_var
>>>    };
>>>    
>>>    /* BTF_KIND_DATASEC is followed by VLEN struct btf_var_secinfo entries,
>>> -   which describe all BTF_KIND_VAR types contained in the section.  */
>>> +   which describe all BTF_KIND_VAR or extern BTF_KIND_FUNC types contained
>>> +   in the section.  */
>>>    struct btf_var_secinfo
>>>    {
>>> -  uint32_t type;	/* Type of variable.  */
>>> -  uint32_t offset;	/* In-section offset of variable (in bytes).  */
>>> -  uint32_t size;	/* Size (in bytes) of variable.  */
>>> +  uint32_t type;	/* Type of BTF_KIND_VAR or BTF_KIND_FUNC item.  */
>>> +  uint32_t offset;	/* In-section offset (in bytes) of item.  */
>>> +  uint32_t size;	/* Size (in bytes) of item.  */
>>>    };
>>>    
>>>    /* BTF_KIND_ENUM64 is followed by VLEN struct btf_enum64 entries,
>>