diff mbox series

btf: Emit labels in DATASEC bts_offset entries.

Message ID 20240326142852.418737-1-cupertino.miranda@oracle.com
State New
Headers show
Series btf: Emit labels in DATASEC bts_offset entries. | expand

Commit Message

Cupertino Miranda March 26, 2024, 2:28 p.m. UTC
Hi everyone,

This patch is an expected fix for the issue reported by systemd in:
https://github.com/systemd/systemd/issues/31888
Also, Jose Marchesi opened the following bugzilla to report it:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114431

Please notice that the function created was inspired by existing code
in the function btf_asm_type_ref.

Looking forward to your comments.

Cupertino


GCC was defining bts_offset entry to always contain 0.
When comparing with clang, the same entry is instead a label to the
respective variable or function. The assembler emits relocations for
those labels.

gcc/ChangeLog:
	PR target/114431
	* btfout.cc (get_name_for_datasec_entry): Add function.
	(btf_asm_datasec_entry): Print label when possible.

gcc/testsuite/ChangeLog:
	gcc.dg/debug/btf/btf-datasec-1.c: Correct for new
	implementation.
	gcc.dg/debug/btf/btf-datasec-2.c: Likewise
	gcc.dg/debug/btf/btf-pr106773.c: Likewise
---
 gcc/btfout.cc                                 | 30 ++++++++++++++++++-
 .../gcc.dg/debug/btf/btf-datasec-1.c          |  6 ++--
 .../gcc.dg/debug/btf/btf-datasec-2.c          |  7 +++--
 gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c |  2 +-
 4 files changed, 39 insertions(+), 6 deletions(-)

Comments

David Faust March 26, 2024, 3:55 p.m. UTC | #1
On 3/26/24 07:28, Cupertino Miranda wrote:
> Hi everyone,
> 
> This patch is an expected fix for the issue reported by systemd in:
> https://github.com/systemd/systemd/issues/31888
> Also, Jose Marchesi opened the following bugzilla to report it:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114431
> 
> Please notice that the function created was inspired by existing code
> in the function btf_asm_type_ref.
> 
> Looking forward to your comments.

OK, LGTM.
Thanks for the fix!

> 
> Cupertino
> 
> 
> GCC was defining bts_offset entry to always contain 0.
> When comparing with clang, the same entry is instead a label to the
> respective variable or function. The assembler emits relocations for
> those labels.
> 
> gcc/ChangeLog:
> 	PR target/114431
> 	* btfout.cc (get_name_for_datasec_entry): Add function.
> 	(btf_asm_datasec_entry): Print label when possible.
> 
> gcc/testsuite/ChangeLog:
> 	gcc.dg/debug/btf/btf-datasec-1.c: Correct for new
> 	implementation.
> 	gcc.dg/debug/btf/btf-datasec-2.c: Likewise
> 	gcc.dg/debug/btf/btf-pr106773.c: Likewise
> ---
>  gcc/btfout.cc                                 | 30 ++++++++++++++++++-
>  .../gcc.dg/debug/btf/btf-datasec-1.c          |  6 ++--
>  .../gcc.dg/debug/btf/btf-datasec-2.c          |  7 +++--
>  gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c |  2 +-
>  4 files changed, 39 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
> index e63289bc554..5f708630e04 100644
> --- a/gcc/btfout.cc
> +++ b/gcc/btfout.cc
> @@ -1014,13 +1014,41 @@ btf_asm_func_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd, ctf_id_t id)
>    btf_asm_type_ref ("btt_type", ctfc, get_btf_id (ref_id));
>  }
>  
> +/* Collect the name for the DATASEC reference required to be output as a
> +   symbol. */
> +
> +static const char *
> +get_name_for_datasec_entry (ctf_container_ref ctfc, ctf_id_t ref_id)
> +{
> +  if (ref_id >= num_types_added + 1
> +      && ref_id < num_types_added + num_vars_added + 1)
> +    {
> +      /* Ref to a variable.  Should only appear in DATASEC entries.  */
> +      ctf_id_t var_id = btf_relative_var_id (ref_id);
> +      ctf_dvdef_ref dvd = ctfc->ctfc_vars_list[var_id];
> +      return dvd->dvd_name;
> +    }
> +  else if (ref_id >= num_types_added + num_vars_added + 1)
> +    {
> +      /* Ref to a FUNC record.  */
> +      size_t func_id = btf_relative_func_id (ref_id);
> +      ctf_dtdef_ref ref_type = (*funcs)[func_id];
> +      return get_btf_type_name (ref_type);
> +    }
> +  return NULL;
> +}
> +
>  /* Asm'out a variable entry following a BTF_KIND_DATASEC.  */
>  
>  static void
>  btf_asm_datasec_entry (ctf_container_ref ctfc, struct btf_var_secinfo info)
>  {
> +  const char *symbol_name = get_name_for_datasec_entry (ctfc, info.type);
>    btf_asm_type_ref ("bts_type", ctfc, info.type);
> -  dw2_asm_output_data (4, info.offset, "bts_offset");
> +  if (symbol_name == NULL)
> +    dw2_asm_output_data (4, info.offset, "bts_offset");
> +  else
> +    dw2_asm_output_offset (4, symbol_name, NULL, "bts_offset");
>    dw2_asm_output_data (4, info.size, "bts_size");
>  }
>  
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
> index 77df88648c5..8557c38c20d 100644
> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
> @@ -19,8 +19,10 @@
>  /* { dg-final { scan-assembler-times "0xf000003\[\t \]+\[^\n\]*btt_info" 2 } } */
>  /* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>  
> -/* The offset entry for each variable in a DATSEC should be 0 at compile time.  */
> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 7 } } */
> +/* The offset entry for each variable in a DATSEC should contain a label.  */
> +/* { dg-final { scan-assembler-times ".4byte\[\t \]\[a-e\]\[\t \]+\[^\n\]*bts_offset" 5 } } */
> +/* { dg-final { scan-assembler-times "my_cstruct\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "bigarr\[\t \]+\[^\n\]*bts_offset" 1 } } */
>  
>  /* Check that strings for each DATASEC have been added to the BTF string table.  */
>  /* { dg-final { scan-assembler-times "ascii \".data.0\"\[\t \]+\[^\n\]*btf_aux_string" 1 } } */
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> index d6f3358d6fb..857d830e446 100644
> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
> @@ -9,8 +9,11 @@
>  /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.foo_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>  /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.bar_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>  
> -/* Function entries should have offset and size of 0 at compile time.  */
> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
> +/* Function entries should have offset with a label and size of 0 at compile time.  */
> +/* { dg-final { scan-assembler-times "chacha\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "bar\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +
>  /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>  
>  extern int foo (int a) __attribute__((section(".foo_sec")));
> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> index 940f129408d..c06220eb520 100644
> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
> @@ -11,7 +11,7 @@
>  
>  /* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>  
> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
> +/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
>  /* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>  
>  extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
Cupertino Miranda March 26, 2024, 4:21 p.m. UTC | #2
David Faust writes:

> On 3/26/24 07:28, Cupertino Miranda wrote:
>> Hi everyone,
>>
>> This patch is an expected fix for the issue reported by systemd in:
>> https://github.com/systemd/systemd/issues/31888
>> Also, Jose Marchesi opened the following bugzilla to report it:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114431
>>
>> Please notice that the function created was inspired by existing code
>> in the function btf_asm_type_ref.
>>
>> Looking forward to your comments.
>
> OK, LGTM.
> Thanks for the fix!

Pushed! Thanks!

>>
>> Cupertino
>>
>>
>> GCC was defining bts_offset entry to always contain 0.
>> When comparing with clang, the same entry is instead a label to the
>> respective variable or function. The assembler emits relocations for
>> those labels.
>>
>> gcc/ChangeLog:
>> 	PR target/114431
>> 	* btfout.cc (get_name_for_datasec_entry): Add function.
>> 	(btf_asm_datasec_entry): Print label when possible.
>>
>> gcc/testsuite/ChangeLog:
>> 	gcc.dg/debug/btf/btf-datasec-1.c: Correct for new
>> 	implementation.
>> 	gcc.dg/debug/btf/btf-datasec-2.c: Likewise
>> 	gcc.dg/debug/btf/btf-pr106773.c: Likewise
>> ---
>>  gcc/btfout.cc                                 | 30 ++++++++++++++++++-
>>  .../gcc.dg/debug/btf/btf-datasec-1.c          |  6 ++--
>>  .../gcc.dg/debug/btf/btf-datasec-2.c          |  7 +++--
>>  gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c |  2 +-
>>  4 files changed, 39 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/btfout.cc b/gcc/btfout.cc
>> index e63289bc554..5f708630e04 100644
>> --- a/gcc/btfout.cc
>> +++ b/gcc/btfout.cc
>> @@ -1014,13 +1014,41 @@ btf_asm_func_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd, ctf_id_t id)
>>    btf_asm_type_ref ("btt_type", ctfc, get_btf_id (ref_id));
>>  }
>>
>> +/* Collect the name for the DATASEC reference required to be output as a
>> +   symbol. */
>> +
>> +static const char *
>> +get_name_for_datasec_entry (ctf_container_ref ctfc, ctf_id_t ref_id)
>> +{
>> +  if (ref_id >= num_types_added + 1
>> +      && ref_id < num_types_added + num_vars_added + 1)
>> +    {
>> +      /* Ref to a variable.  Should only appear in DATASEC entries.  */
>> +      ctf_id_t var_id = btf_relative_var_id (ref_id);
>> +      ctf_dvdef_ref dvd = ctfc->ctfc_vars_list[var_id];
>> +      return dvd->dvd_name;
>> +    }
>> +  else if (ref_id >= num_types_added + num_vars_added + 1)
>> +    {
>> +      /* Ref to a FUNC record.  */
>> +      size_t func_id = btf_relative_func_id (ref_id);
>> +      ctf_dtdef_ref ref_type = (*funcs)[func_id];
>> +      return get_btf_type_name (ref_type);
>> +    }
>> +  return NULL;
>> +}
>> +
>>  /* Asm'out a variable entry following a BTF_KIND_DATASEC.  */
>>
>>  static void
>>  btf_asm_datasec_entry (ctf_container_ref ctfc, struct btf_var_secinfo info)
>>  {
>> +  const char *symbol_name = get_name_for_datasec_entry (ctfc, info.type);
>>    btf_asm_type_ref ("bts_type", ctfc, info.type);
>> -  dw2_asm_output_data (4, info.offset, "bts_offset");
>> +  if (symbol_name == NULL)
>> +    dw2_asm_output_data (4, info.offset, "bts_offset");
>> +  else
>> +    dw2_asm_output_offset (4, symbol_name, NULL, "bts_offset");
>>    dw2_asm_output_data (4, info.size, "bts_size");
>>  }
>>
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
>> index 77df88648c5..8557c38c20d 100644
>> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
>> @@ -19,8 +19,10 @@
>>  /* { dg-final { scan-assembler-times "0xf000003\[\t \]+\[^\n\]*btt_info" 2 } } */
>>  /* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>>
>> -/* The offset entry for each variable in a DATSEC should be 0 at compile time.  */
>> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 7 } } */
>> +/* The offset entry for each variable in a DATSEC should contain a label.  */
>> +/* { dg-final { scan-assembler-times ".4byte\[\t \]\[a-e\]\[\t \]+\[^\n\]*bts_offset" 5 } } */
>> +/* { dg-final { scan-assembler-times "my_cstruct\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "bigarr\[\t \]+\[^\n\]*bts_offset" 1 } } */
>>
>>  /* Check that strings for each DATASEC have been added to the BTF string table.  */
>>  /* { dg-final { scan-assembler-times "ascii \".data.0\"\[\t \]+\[^\n\]*btf_aux_string" 1 } } */
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> index d6f3358d6fb..857d830e446 100644
>> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
>> @@ -9,8 +9,11 @@
>>  /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.foo_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>>  /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.bar_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
>>
>> -/* Function entries should have offset and size of 0 at compile time.  */
>> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
>> +/* Function entries should have offset with a label and size of 0 at compile time.  */
>> +/* { dg-final { scan-assembler-times "chacha\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "bar\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +
>>  /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
>>
>>  extern int foo (int a) __attribute__((section(".foo_sec")));
>> diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> index 940f129408d..c06220eb520 100644
>> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
>> @@ -11,7 +11,7 @@
>>
>>  /* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
>>
>> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
>> +/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
>>  /* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
>>
>>  extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));
Jakub Jelinek March 27, 2024, 10:18 a.m. UTC | #3
On Tue, Mar 26, 2024 at 02:28:52PM +0000, Cupertino Miranda wrote:
> GCC was defining bts_offset entry to always contain 0.
> When comparing with clang, the same entry is instead a label to the
> respective variable or function. The assembler emits relocations for
> those labels.
> 
> gcc/ChangeLog:
> 	PR target/114431
> 	* btfout.cc (get_name_for_datasec_entry): Add function.
> 	(btf_asm_datasec_entry): Print label when possible.
> 
> gcc/testsuite/ChangeLog:
> 	gcc.dg/debug/btf/btf-datasec-1.c: Correct for new
> 	implementation.
> 	gcc.dg/debug/btf/btf-datasec-2.c: Likewise
> 	gcc.dg/debug/btf/btf-pr106773.c: Likewise

For next time, missing dot after Likewise

> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
> @@ -19,8 +19,10 @@
>  /* { dg-final { scan-assembler-times "0xf000003\[\t \]+\[^\n\]*btt_info" 2 } } */
>  /* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>  
> -/* The offset entry for each variable in a DATSEC should be 0 at compile time.  */
> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 7 } } */
> +/* The offset entry for each variable in a DATSEC should contain a label.  */
> +/* { dg-final { scan-assembler-times ".4byte\[\t \]\[a-e\]\[\t \]+\[^\n\]*bts_offset" 5 } } */

Where has this been tested?
4byte is used only on some targets, what exact assembler directive is used
for 4byte unaligned data is heavily target dependent.
git grep define.*TARGET_ASM_UNALIGNED_SI_OP
gcc/config/alpha/alpha.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.align 0\n\t.long\t"
gcc/config/avr/avr.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
gcc/config/cris/cris.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
gcc/config/csky/csky.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
gcc/config/i386/i386.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
gcc/config/ia64/ia64.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\tdata4.ua\t"
gcc/config/m68k/m68k.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
gcc/config/mcore/mcore.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
gcc/config/pa/pa.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
gcc/config/rs6000/rs6000.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.vbyte\t4,"
gcc/config/rs6000/rs6000.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
gcc/config/sh/sh.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.ualong\t"
gcc/config/sparc/sparc.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.uaword\t"
gcc/target-def.h:#define TARGET_ASM_UNALIGNED_SI_OP "\t.4byte\t"
gcc/target-def.h:#define TARGET_ASM_UNALIGNED_SI_OP NULL
Now
git grep 'define[[:blank:]]*TARGET_ASM_ALIGNED_SI_OP' | grep '/\(cris\|i386\|m68k\|pa\)/'
gcc/config/cris/cris.cc:#define TARGET_ASM_ALIGNED_SI_OP "\t.dword\t"
gcc/config/i386/i386.cc:#define TARGET_ASM_ALIGNED_SI_OP ASM_LONG
gcc/config/m68k/m68k.cc:#define TARGET_ASM_ALIGNED_SI_OP "\tlong\t"
gcc/config/m68k/m68k.cc:#define TARGET_ASM_ALIGNED_SI_OP "\tdc.l\t"
gcc/config/pa/pa.cc:#define TARGET_ASM_ALIGNED_SI_OP "\t.word\t"
git grep 'define[[:blank:]]*ASM_LONG'
gcc/config/i386/att.h:#define ASM_LONG "\t.long\t"
gcc/config/i386/bsd.h:#define ASM_LONG "\t.long\t"
gcc/config/i386/darwin.h:#define ASM_LONG "\t.long\t"

So, if you want to handle it for all arches, instead of .4byte\[\t \] you'd need to
use
(?:(?:.4byte|.long|data4.ua|.ualong|.uaword|.dword|long|dc.l|.word)\[\t \]|.vbyte\t4,\[\t \]?)
or so.

BTW,
/* { dg-options "-O0 -gbtf -dA" } */
/* { dg-options "-O0 -gbtf -dA -msdata=none" { target { { powerpc*-*-* } && ilp32 } } } */
/* { dg-options "-O0 -gbtf -dA -msmall-data-limit=0" { target { riscv*-*-* } } } */
/* { dg-options "-O0 -gbtf -dA -G0" { target { nios2-*-* } } } */
would be perhaps right for test written 2 decades ago, but for a test
written 3 years ago is something that shouldn't have passed patch review.
This should be done with
/* { dg-options "-O0 -gbtf -dA" } */
/* { dg-additional-options "-msdata=none" { target { { powerpc*-*-* } && ilp32 } } } */
/* { dg-additional-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
/* { dg-additional-options "-G0" { target { nios2-*-* } } } */

btf-cvr-quals-1.c test suffers from that too.

	Jakub
Cupertino Miranda March 27, 2024, 7:14 p.m. UTC | #4
Hi Jakub,

Thanks for the patch and appologies for the results regression.

Cupertino

Jakub Jelinek writes:

> On Tue, Mar 26, 2024 at 02:28:52PM +0000, Cupertino Miranda wrote:
>> GCC was defining bts_offset entry to always contain 0.
>> When comparing with clang, the same entry is instead a label to the
>> respective variable or function. The assembler emits relocations for
>> those labels.
>>
>> gcc/ChangeLog:
>> 	PR target/114431
>> 	* btfout.cc (get_name_for_datasec_entry): Add function.
>> 	(btf_asm_datasec_entry): Print label when possible.
>>
>> gcc/testsuite/ChangeLog:
>> 	gcc.dg/debug/btf/btf-datasec-1.c: Correct for new
>> 	implementation.
>> 	gcc.dg/debug/btf/btf-datasec-2.c: Likewise
>> 	gcc.dg/debug/btf/btf-pr106773.c: Likewise
>
> For next time, missing dot after Likewise
>
>> --- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
>> +++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
>> @@ -19,8 +19,10 @@
>>  /* { dg-final { scan-assembler-times "0xf000003\[\t \]+\[^\n\]*btt_info" 2 } } */
>>  /* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
>>
>> -/* The offset entry for each variable in a DATSEC should be 0 at compile time.  */
>> -/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 7 } } */
>> +/* The offset entry for each variable in a DATSEC should contain a label.  */
>> +/* { dg-final { scan-assembler-times ".4byte\[\t \]\[a-e\]\[\t \]+\[^\n\]*bts_offset" 5 } } */
>
> Where has this been tested?
> 4byte is used only on some targets, what exact assembler directive is used
> for 4byte unaligned data is heavily target dependent.
> git grep define.*TARGET_ASM_UNALIGNED_SI_OP
> gcc/config/alpha/alpha.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.align 0\n\t.long\t"
> gcc/config/avr/avr.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
> gcc/config/cris/cris.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
> gcc/config/csky/csky.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
> gcc/config/i386/i386.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
> gcc/config/ia64/ia64.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\tdata4.ua\t"
> gcc/config/m68k/m68k.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
> gcc/config/mcore/mcore.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
> gcc/config/pa/pa.cc:#define TARGET_ASM_UNALIGNED_SI_OP TARGET_ASM_ALIGNED_SI_OP
> gcc/config/rs6000/rs6000.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.vbyte\t4,"
> gcc/config/rs6000/rs6000.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.long\t"
> gcc/config/sh/sh.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.ualong\t"
> gcc/config/sparc/sparc.cc:#define TARGET_ASM_UNALIGNED_SI_OP "\t.uaword\t"
> gcc/target-def.h:#define TARGET_ASM_UNALIGNED_SI_OP "\t.4byte\t"
> gcc/target-def.h:#define TARGET_ASM_UNALIGNED_SI_OP NULL
> Now
> git grep 'define[[:blank:]]*TARGET_ASM_ALIGNED_SI_OP' | grep '/\(cris\|i386\|m68k\|pa\)/'
> gcc/config/cris/cris.cc:#define TARGET_ASM_ALIGNED_SI_OP "\t.dword\t"
> gcc/config/i386/i386.cc:#define TARGET_ASM_ALIGNED_SI_OP ASM_LONG
> gcc/config/m68k/m68k.cc:#define TARGET_ASM_ALIGNED_SI_OP "\tlong\t"
> gcc/config/m68k/m68k.cc:#define TARGET_ASM_ALIGNED_SI_OP "\tdc.l\t"
> gcc/config/pa/pa.cc:#define TARGET_ASM_ALIGNED_SI_OP "\t.word\t"
> git grep 'define[[:blank:]]*ASM_LONG'
> gcc/config/i386/att.h:#define ASM_LONG "\t.long\t"
> gcc/config/i386/bsd.h:#define ASM_LONG "\t.long\t"
> gcc/config/i386/darwin.h:#define ASM_LONG "\t.long\t"
>
> So, if you want to handle it for all arches, instead of .4byte\[\t \] you'd need to
> use
> (?:(?:.4byte|.long|data4.ua|.ualong|.uaword|.dword|long|dc.l|.word)\[\t \]|.vbyte\t4,\[\t \]?)
> or so.
>
> BTW,
> /* { dg-options "-O0 -gbtf -dA" } */
> /* { dg-options "-O0 -gbtf -dA -msdata=none" { target { { powerpc*-*-* } && ilp32 } } } */
> /* { dg-options "-O0 -gbtf -dA -msmall-data-limit=0" { target { riscv*-*-* } } } */
> /* { dg-options "-O0 -gbtf -dA -G0" { target { nios2-*-* } } } */
> would be perhaps right for test written 2 decades ago, but for a test
> written 3 years ago is something that shouldn't have passed patch review.
> This should be done with
> /* { dg-options "-O0 -gbtf -dA" } */
> /* { dg-additional-options "-msdata=none" { target { { powerpc*-*-* } && ilp32 } } } */
> /* { dg-additional-options "-msmall-data-limit=0" { target { riscv*-*-* } } } */
> /* { dg-additional-options "-G0" { target { nios2-*-* } } } */
>
> btf-cvr-quals-1.c test suffers from that too.
>
> 	Jakub
diff mbox series

Patch

diff --git a/gcc/btfout.cc b/gcc/btfout.cc
index e63289bc554..5f708630e04 100644
--- a/gcc/btfout.cc
+++ b/gcc/btfout.cc
@@ -1014,13 +1014,41 @@  btf_asm_func_type (ctf_container_ref ctfc, ctf_dtdef_ref dtd, ctf_id_t id)
   btf_asm_type_ref ("btt_type", ctfc, get_btf_id (ref_id));
 }
 
+/* Collect the name for the DATASEC reference required to be output as a
+   symbol. */
+
+static const char *
+get_name_for_datasec_entry (ctf_container_ref ctfc, ctf_id_t ref_id)
+{
+  if (ref_id >= num_types_added + 1
+      && ref_id < num_types_added + num_vars_added + 1)
+    {
+      /* Ref to a variable.  Should only appear in DATASEC entries.  */
+      ctf_id_t var_id = btf_relative_var_id (ref_id);
+      ctf_dvdef_ref dvd = ctfc->ctfc_vars_list[var_id];
+      return dvd->dvd_name;
+    }
+  else if (ref_id >= num_types_added + num_vars_added + 1)
+    {
+      /* Ref to a FUNC record.  */
+      size_t func_id = btf_relative_func_id (ref_id);
+      ctf_dtdef_ref ref_type = (*funcs)[func_id];
+      return get_btf_type_name (ref_type);
+    }
+  return NULL;
+}
+
 /* Asm'out a variable entry following a BTF_KIND_DATASEC.  */
 
 static void
 btf_asm_datasec_entry (ctf_container_ref ctfc, struct btf_var_secinfo info)
 {
+  const char *symbol_name = get_name_for_datasec_entry (ctfc, info.type);
   btf_asm_type_ref ("bts_type", ctfc, info.type);
-  dw2_asm_output_data (4, info.offset, "bts_offset");
+  if (symbol_name == NULL)
+    dw2_asm_output_data (4, info.offset, "bts_offset");
+  else
+    dw2_asm_output_offset (4, symbol_name, NULL, "bts_offset");
   dw2_asm_output_data (4, info.size, "bts_size");
 }
 
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
index 77df88648c5..8557c38c20d 100644
--- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-1.c
@@ -19,8 +19,10 @@ 
 /* { dg-final { scan-assembler-times "0xf000003\[\t \]+\[^\n\]*btt_info" 2 } } */
 /* { dg-final { scan-assembler-times "0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
 
-/* The offset entry for each variable in a DATSEC should be 0 at compile time.  */
-/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 7 } } */
+/* The offset entry for each variable in a DATSEC should contain a label.  */
+/* { dg-final { scan-assembler-times ".4byte\[\t \]\[a-e\]\[\t \]+\[^\n\]*bts_offset" 5 } } */
+/* { dg-final { scan-assembler-times "my_cstruct\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "bigarr\[\t \]+\[^\n\]*bts_offset" 1 } } */
 
 /* Check that strings for each DATASEC have been added to the BTF string table.  */
 /* { dg-final { scan-assembler-times "ascii \".data.0\"\[\t \]+\[^\n\]*btf_aux_string" 1 } } */
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
index d6f3358d6fb..857d830e446 100644
--- a/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-datasec-2.c
@@ -9,8 +9,11 @@ 
 /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.foo_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000001\[\t \]+\[^\n\]*btt_info" 1 } } */
 /* { dg-final { scan-assembler-times " BTF_KIND_DATASEC '.bar_sec'\[\\r\\n\]+\[^\\r\\n\]*0xf000002\[\t \]+\[^\n\]*btt_info" 1 } } */
 
-/* Function entries should have offset and size of 0 at compile time.  */
-/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 3 } } */
+/* Function entries should have offset with a label and size of 0 at compile time.  */
+/* { dg-final { scan-assembler-times "chacha\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "bar\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
+
 /* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_size" 3 } } */
 
 extern int foo (int a) __attribute__((section(".foo_sec")));
diff --git a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
index 940f129408d..c06220eb520 100644
--- a/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
+++ b/gcc/testsuite/gcc.dg/debug/btf/btf-pr106773.c
@@ -11,7 +11,7 @@ 
 
 /* { dg-final { scan-assembler-times "ascii \"foo.0\"\[\t \]+\[^\n\]*btf_string" 1 } } */
 
-/* { dg-final { scan-assembler-times "0\[\t \]+\[^\n\]*bts_offset" 1 } } */
+/* { dg-final { scan-assembler-times "foo\[\t \]+\[^\n\]*bts_offset" 1 } } */
 /* { dg-final { scan-assembler-times "1\[\t \]+\[^\n\]*bts_size" 1 } } */
 
 extern const void foo __attribute__((weak)) __attribute__((section (".ksyms")));