diff mbox series

btf: avoid wrong DATASEC entries for extern vars [PR112849]

Message ID 20231204234715.9773-1-david.faust@oracle.com
State New
Headers
Series btf: avoid wrong DATASEC entries for extern vars [PR112849] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

David Faust Dec. 4, 2023, 11:47 p.m. UTC 
  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.

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

Comments

Indu Bhagat Dec. 5, 2023, 9:28 p.m. UTC | #1 
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" ?

> +/* { 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 } } */
David Faust Dec. 5, 2023, 10:09 p.m. UTC | #2 
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 } } */
>
diff mbox series

Patch

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.  */
+/* { 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 } } */