public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Faust <david.faust@oracle.com>
To: Indu Bhagat <indu.bhagat@oracle.com>
Cc: jose.marchesi@oracle.com, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773]
Date: Mon, 12 Dec 2022 12:47:39 -0800	[thread overview]
Message-ID: <261c7cf6-0d5f-bc3b-5d5a-ae5d6c944c0b@oracle.com> (raw)
In-Reply-To: <3c332b0b-5193-48e7-7790-1b02d87f33a1@oracle.com>



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,
> 

  reply	other threads:[~2022-12-12 20:47 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-07 20:57 [PATCH 0/3] btf: fix BTF for extern items [PR106773] David Faust
2022-12-07 20:57 ` [PATCH 1/3] btf: add 'extern' linkage for variables [PR106773] David Faust
2022-12-09  6:55   ` Indu Bhagat
2022-12-12 20:47     ` David Faust [this message]
2022-12-13  6:15       ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 2/3] btf: fix 'extern const void' " David Faust
2022-12-09  7:34   ` Indu Bhagat
2022-12-12 20:59     ` David Faust
2022-12-13  6:11       ` Indu Bhagat
2022-12-07 20:57 ` [PATCH 3/3] btf: correct generation for extern funcs [PR106773] David Faust
2022-12-09  7:36   ` Indu Bhagat
2022-12-12 20:31     ` David Faust
2022-12-13  6:12       ` Indu Bhagat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=261c7cf6-0d5f-bc3b-5d5a-ae5d6c944c0b@oracle.com \
    --to=david.faust@oracle.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=indu.bhagat@oracle.com \
    --cc=jose.marchesi@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).