On 12/5/23 13:28, Indu Bhagat wrote:
> On 12/4/23 15:47, David Faust wrote:
>> The process of creating BTF_KIND_DATASEC records involves iterating
>> through variable declarations, determining which section they will be
>> placed in, and creating an entry in the appropriate DATASEC record
>> accordingly.
>>
>> For variables without e.g. an explicit __attribute__((section)), we use
>> categorize_decl_for_section () to identify the appropriate named section
>> and corresponding BTF_KIND_DATASEC record.
>>
>> This was incorrectly being done for 'extern' variable declarations as
>> well as non-extern ones, which meant that extern variable declarations
>> could result in BTF_KIND_DATASEC entries claiming the variable is
>> allocated in some section such as '.bss' without any knowledge whether
>> that is actually true. That resulted in errors building the Linux kernel
>> BPF selftests.
>>
>> This patch corrects btf_collect_datasec () to avoid assuming a section
>> for extern variables, and only emit BTF_KIND_DATASEC entries for them if
>> they have a known section.
>>
>> Bootstrapped + tested on x86_64-linux-gnu.
>> Tested on x86_64-linux-gnu host for bpf-unknown-none.
>>
>
> One comment below.
>
> LGTM, otherwise.
> Thanks
>
>> gcc/
>> PR debug/112849
>> * btfout.cc (btf_collect_datasec): Avoid incorrectly creating an
>> entry in a BTF_KIND_DATASEC record for extern variable decls without
>> a known section.
>>
>> gcc/testsuite/
>> PR debug/112849
>> * gcc.dg/debug/btf/btf-datasec-3.c: New test.
>> ---
>> gcc/btfout.cc | 10 ++++++-
>> .../gcc.dg/debug/btf/btf-datasec-3.c | 27 +++++++++++++++++++
>> 2 files changed, 36 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index a5e0d640e19..db4f1084f85 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -486,7 +486,15 @@ btf_collect_datasec (ctf_container_ref ctfc)
>>
>> /* Mark extern variables. */
>> if (DECL_EXTERNAL (node->decl))
>> - dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN;
>> + {
>> + dvd->dvd_visibility = BTF_VAR_GLOBAL_EXTERN;
>> +
>> + /* PR112849: avoid assuming a section for extern decls without
>> + an explicit section, which would result in incorrectly
>> + emitting a BTF_KIND_DATASEC entry for them. */
>> + if (node->get_section () == NULL)
>> + continue;
>> + }
>>
>> const char *section_name = get_section_name (node);
>> if (section_name == NULL)
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c
>> b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c
>> new file mode 100644
>> index 00000000000..3c1c7a28c2a
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-3.c
>> @@ -0,0 +1,27 @@
>> +/* PR debug/112849
>> + Test that we do not incorrectly create BTF_KIND_DATASEC entries for
>> + extern decls with no known section. */
>> +
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -gbtf -dA" } */
>> +
>> +extern int VERSION __attribute__((section (".version")));
>> +
>> +extern int test_bss1;
>> +extern int test_data1;
>> +
>> +int test_bss2;
>> +int test_data2 = 2;
>> +
>> +int
>> +foo (void)
>> +{
>> + test_bss2 = VERSION;
>> + return test_bss1 + test_data1 + test_data2;
>> +}
>> +
>> +/* There should only be a DATASEC entries for VERSION out of the extern
>> decls. */
>
> The statement is unclear as is. Perhaps you wanted to say "There should
> only be 3 DATASEC entries; including one for VERSION even though it is
> extern decl" ?
Thanks. I reworded parts of that once or twice ended up with a garbled mess.
Changed to:
/* There should be 3 DATASEC entries total. Of the extern decls, only VERSION
has a known section; entries are not created for the other two. */
and pushed.
>
>> +/* { dg-final { scan-assembler-times "bts_type" 3 } } */
>> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR
>> 'test_data2'\\)" 1 } } */
>> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR
>> 'test_bss2'\\)" 1 } } */
>> +/* { dg-final { scan-assembler-times "bts_type: \\(BTF_KIND_VAR
>> 'VERSION'\\)" 1 } } */
>
