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

[Indu Bhagat <indu.bhagat@oracle.com>]

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,