| Message ID | 20221207205734.9287-2-david.faust@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | btf: fix BTF for extern items [PR106773] | expand |
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,
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, >
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, >>
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,