[2/2] Corrected pr25521.c target matching.

Message ID 20221202175225.2780-3-cupertino.miranda@oracle.com
State New
Headers
Series [1/2] select .rodata for const volatile variables. |

Commit Message

Cupertino Miranda Dec. 2, 2022, 5:52 p.m. UTC
  This commit is a follow up of bugzilla #107181.

The commit /a0aafbc/ changed the default implementation of the
SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
placement of `const volatile' objects.

However, the following targets use target-specific selection functions
and they choke on the testcase pr25521.c:

 *rx - target sets its const variables as '.section C,"a",@progbits'.

 *powerpc - its 32bit version is eager to allocate globals in .sdata
            sections.

Normally, one can expect for the variable to be allocated in .srodata,
however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
'targetm.have_srodata_section == false' and the code in
categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.

  /* If the target uses small data sections, select it.  */
  else if (targetm.in_small_data_p (decl))
    {
      if (ret == SECCAT_BSS)
	ret = SECCAT_SBSS;
      else if targetm.have_srodata_section && ret == SECCAT_RODATA)
	ret = SECCAT_SRODATA;
      else
	ret = SECCAT_SDATA;
    }

LLVM compiler does not generate .sdata symbols at all, having different code
generation even for non const volatile symbols.
Targets that for acceptable reasons could not match the LLVM generated code
were marked as XFAIL.

gcc/testsuite/ChangeLog:

	* lib/target-supports.exp: Added
	  check_effective_target_const_volatile_readonly_section.
	* gcc.dg/pr25521.c: Added XFAIL.
---
 gcc/testsuite/gcc.dg/pr25521.c        |  3 +--
 gcc/testsuite/lib/target-supports.exp | 12 ++++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)
  

Comments

Jeff Law Dec. 5, 2022, 6:47 p.m. UTC | #1
On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
> This commit is a follow up of bugzilla #107181.
> 
> The commit /a0aafbc/ changed the default implementation of the
> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
> placement of `const volatile' objects.
> 
> However, the following targets use target-specific selection functions
> and they choke on the testcase pr25521.c:
> 
>   *rx - target sets its const variables as '.section C,"a",@progbits'.
That's presumably a constant section.  We should instead twiddle the 
test to recognize that section.

> 
>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>              sections.
> 
> Normally, one can expect for the variable to be allocated in .srodata,
> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
> 'targetm.have_srodata_section == false' and the code in
> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
> 
>    /* If the target uses small data sections, select it.  */
>    else if (targetm.in_small_data_p (decl))
>      {
>        if (ret == SECCAT_BSS)
> 	ret = SECCAT_SBSS;
>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
> 	ret = SECCAT_SRODATA;
>        else
> 	ret = SECCAT_SDATA;
>      }
I'd just skip the test for 32bit ppc.  There should be suitable 
effective-target tests you can use.

jeff
  
Cupertino Miranda Dec. 7, 2022, 3:45 p.m. UTC | #2
> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>> This commit is a follow up of bugzilla #107181.
>> The commit /a0aafbc/ changed the default implementation of the
>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>> placement of `const volatile' objects.
>> However, the following targets use target-specific selection functions
>> and they choke on the testcase pr25521.c:
>>   *rx - target sets its const variables as '.section C,"a",@progbits'.
> That's presumably a constant section.  We should instead twiddle the test to
> recognize that section.

Although @progbits is indeed a constant section, I believe it is
more interesting to detect if the `rx' starts selecting more
standard sections instead of the current @progbits.
That was the reason why I opted to XFAIL instead of PASSing it.
Can I keep it as such ?

>
>>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>>              sections.
>> Normally, one can expect for the variable to be allocated in .srodata,
>> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
>> 'targetm.have_srodata_section == false' and the code in
>> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
>>    /* If the target uses small data sections, select it.  */
>>    else if (targetm.in_small_data_p (decl))
>>      {
>>        if (ret == SECCAT_BSS)
>> 	ret = SECCAT_SBSS;
>>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
>> 	ret = SECCAT_SRODATA;
>>        else
>> 	ret = SECCAT_SDATA;
>>      }
> I'd just skip the test for 32bit ppc.  There should be suitable effective-target
> tests you can use.
>
> jeff
  
Cupertino Miranda Dec. 15, 2022, 10:14 a.m. UTC | #3
gentle ping

Cupertino Miranda writes:

>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> This commit is a follow up of bugzilla #107181.
>>> The commit /a0aafbc/ changed the default implementation of the
>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>> placement of `const volatile' objects.
>>> However, the following targets use target-specific selection functions
>>> and they choke on the testcase pr25521.c:
>>>   *rx - target sets its const variables as '.section C,"a",@progbits'.
>> That's presumably a constant section.  We should instead twiddle the test to
>> recognize that section.
>
> Although @progbits is indeed a constant section, I believe it is
> more interesting to detect if the `rx' starts selecting more
> standard sections instead of the current @progbits.
> That was the reason why I opted to XFAIL instead of PASSing it.
> Can I keep it as such ?
>
>>
>>>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>>>              sections.
>>> Normally, one can expect for the variable to be allocated in .srodata,
>>> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
>>> 'targetm.have_srodata_section == false' and the code in
>>> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
>>>    /* If the target uses small data sections, select it.  */
>>>    else if (targetm.in_small_data_p (decl))
>>>      {
>>>        if (ret == SECCAT_BSS)
>>> 	ret = SECCAT_SBSS;
>>>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
>>> 	ret = SECCAT_SRODATA;
>>>        else
>>> 	ret = SECCAT_SDATA;
>>>      }
>> I'd just skip the test for 32bit ppc.  There should be suitable effective-target
>> tests you can use.
>>
>> jeff
  
Cupertino Miranda Dec. 22, 2022, 5:22 p.m. UTC | #4
Cupertino Miranda via Gcc-patches writes:

> gentle ping
>
> Cupertino Miranda writes:
>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>> This commit is a follow up of bugzilla #107181.
>>>> The commit /a0aafbc/ changed the default implementation of the
>>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>>> placement of `const volatile' objects.
>>>> However, the following targets use target-specific selection functions
>>>> and they choke on the testcase pr25521.c:
>>>>   *rx - target sets its const variables as '.section C,"a",@progbits'.
>>> That's presumably a constant section.  We should instead twiddle the test to
>>> recognize that section.
>>
>> Although @progbits is indeed a constant section, I believe it is
>> more interesting to detect if the `rx' starts selecting more
>> standard sections instead of the current @progbits.
>> That was the reason why I opted to XFAIL instead of PASSing it.
>> Can I keep it as such ?
>>
>>>
>>>>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>>>>              sections.
>>>> Normally, one can expect for the variable to be allocated in .srodata,
>>>> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
>>>> 'targetm.have_srodata_section == false' and the code in
>>>> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
>>>>    /* If the target uses small data sections, select it.  */
>>>>    else if (targetm.in_small_data_p (decl))
>>>>      {
>>>>        if (ret == SECCAT_BSS)
>>>> 	ret = SECCAT_SBSS;
>>>>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
>>>> 	ret = SECCAT_SRODATA;
>>>>        else
>>>> 	ret = SECCAT_SDATA;
>>>>      }
>>> I'd just skip the test for 32bit ppc.  There should be suitable effective-target
>>> tests you can use.
>>>
>>> jeff
  
Cupertino Miranda Jan. 2, 2023, 10:43 a.m. UTC | #5
PING PING

Cupertino Miranda writes:

> Cupertino Miranda via Gcc-patches writes:
>
>> gentle ping
>>
>> Cupertino Miranda writes:
>>
>>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>>> This commit is a follow up of bugzilla #107181.
>>>>> The commit /a0aafbc/ changed the default implementation of the
>>>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>>>> placement of `const volatile' objects.
>>>>> However, the following targets use target-specific selection functions
>>>>> and they choke on the testcase pr25521.c:
>>>>>   *rx - target sets its const variables as '.section C,"a",@progbits'.
>>>> That's presumably a constant section.  We should instead twiddle the test to
>>>> recognize that section.
>>>
>>> Although @progbits is indeed a constant section, I believe it is
>>> more interesting to detect if the `rx' starts selecting more
>>> standard sections instead of the current @progbits.
>>> That was the reason why I opted to XFAIL instead of PASSing it.
>>> Can I keep it as such ?
>>>
>>>>
>>>>>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>>>>>              sections.
>>>>> Normally, one can expect for the variable to be allocated in .srodata,
>>>>> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
>>>>> 'targetm.have_srodata_section == false' and the code in
>>>>> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
>>>>>    /* If the target uses small data sections, select it.  */
>>>>>    else if (targetm.in_small_data_p (decl))
>>>>>      {
>>>>>        if (ret == SECCAT_BSS)
>>>>> 	ret = SECCAT_SBSS;
>>>>>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
>>>>> 	ret = SECCAT_SRODATA;
>>>>>        else
>>>>> 	ret = SECCAT_SDATA;
>>>>>      }
>>>> I'd just skip the test for 32bit ppc.  There should be suitable effective-target
>>>> tests you can use.
>>>>
>>>> jeff
  
Cupertino Miranda Jan. 13, 2023, 3:13 p.m. UTC | #6
Cupertino Miranda writes:

>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> This commit is a follow up of bugzilla #107181.
>>> The commit /a0aafbc/ changed the default implementation of the
>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>> placement of `const volatile' objects.
>>> However, the following targets use target-specific selection functions
>>> and they choke on the testcase pr25521.c:
>>>   *rx - target sets its const variables as '.section C,"a",@progbits'.
>> That's presumably a constant section.  We should instead twiddle the test to
>> recognize that section.
>
> Although @progbits is indeed a constant section, I believe it is
> more interesting to detect if the `rx' starts selecting more
> standard sections instead of the current @progbits.
> That was the reason why I opted to XFAIL instead of PASSing it.
> Can I keep it as such ?
>
Jeff: Can you please give me an answer on this ?

Cupertino

>>
>>>   *powerpc - its 32bit version is eager to allocate globals in .sdata
>>>              sections.
>>> Normally, one can expect for the variable to be allocated in .srodata,
>>> however, in case of powerpc-*-* or powerpc64-*-* (with -m32)
>>> 'targetm.have_srodata_section == false' and the code in
>>> categorize_decl_for_section(varasm.cc), forces it to allocate in .sdata.
>>>    /* If the target uses small data sections, select it.  */
>>>    else if (targetm.in_small_data_p (decl))
>>>      {
>>>        if (ret == SECCAT_BSS)
>>> 	ret = SECCAT_SBSS;
>>>        else if targetm.have_srodata_section && ret == SECCAT_RODATA)
>>> 	ret = SECCAT_SRODATA;
>>>        else
>>> 	ret = SECCAT_SDATA;
>>>      }
>> I'd just skip the test for 32bit ppc.  There should be suitable effective-target
>> tests you can use.
>>
>> jeff
  
Jeff Law Jan. 22, 2023, 7:04 p.m. UTC | #7
On 12/7/22 08:45, Cupertino Miranda wrote:
> 
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> This commit is a follow up of bugzilla #107181.
>>> The commit /a0aafbc/ changed the default implementation of the
>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>> placement of `const volatile' objects.
>>> However, the following targets use target-specific selection functions
>>> and they choke on the testcase pr25521.c:
>>>    *rx - target sets its const variables as '.section C,"a",@progbits'.
>> That's presumably a constant section.  We should instead twiddle the test to
>> recognize that section.
> 
> Although @progbits is indeed a constant section, I believe it is
> more interesting to detect if the `rx' starts selecting more
> standard sections instead of the current @progbits.
> That was the reason why I opted to XFAIL instead of PASSing it.
> Can I keep it as such ?
I'm not aware of any ongoing development for that port, so I would not 
let concerns about the rx port changing behavior dominate how we 
approach this problem.

The rx port is using a different name for the section.  That's  valid 
thing to do and to the extent we can, we should support that in the test 
rather than (incorrectly IMHO) xfailing the test just becuase the name 
isn't what we expected.

To avoid over-eagerly matching, I would probably search for "C,"  I 
wouldn't do that for the const or rodata sections as they often have a 
suffix like 1, 2, 4, 8 for different sized rodata sections.

PPC32 is explicitly doing something different and placing those objects 
into an RW section.  So for PPC32 it makes more sense to skip the test 
rather than xfail it.

Jeff
  
Cupertino Miranda Jan. 24, 2023, 12:24 p.m. UTC | #8
Thank you for the comments and suggestions.
I have changed the patch.

Unfortunately in case of rx target I could not make
scan-assembler-symbol-section to match. I believe it is because the
.section and .global entries order is reversed in this target.

Patch in inlined below. looking forward to your comments.

Cupertino
Jeff Law writes:

> On 12/7/22 08:45, Cupertino Miranda wrote:
>>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>> This commit is a follow up of bugzilla #107181.
>>>> The commit /a0aafbc/ changed the default implementation of the
>>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>>> placement of `const volatile' objects.
>>>> However, the following targets use target-specific selection functions
>>>> and they choke on the testcase pr25521.c:
>>>>    *rx - target sets its const variables as '.section C,"a",@progbits'.
>>> That's presumably a constant section.  We should instead twiddle the test to
>>> recognize that section.
>> Although @progbits is indeed a constant section, I believe it is
>> more interesting to detect if the `rx' starts selecting more
>> standard sections instead of the current @progbits.
>> That was the reason why I opted to XFAIL instead of PASSing it.
>> Can I keep it as such ?
> I'm not aware of any ongoing development for that port, so I would not let
> concerns about the rx port changing behavior dominate how we approach this
> problem.
>
> The rx port is using a different name for the section.  That's  valid thing to
> do and to the extent we can, we should support that in the test rather than
> (incorrectly IMHO) xfailing the test just becuase the name isn't what we
> expected.
>
> To avoid over-eagerly matching, I would probably search for "C,"  I wouldn't do
> that for the const or rodata sections as they often have a suffix like 1, 2, 4,
> 8 for different sized rodata sections.
>
> PPC32 is explicitly doing something different and placing those objects into an
> RW section.  So for PPC32 it makes more sense to skip the test rather than xfail
> it.
>
> Jeff
  
Cupertino Miranda Jan. 31, 2023, 9:10 a.m. UTC | #9
Cupertino Miranda via Gcc-patches writes:

> Thank you for the comments and suggestions.
> I have changed the patch.
>
> Unfortunately in case of rx target I could not make
> scan-assembler-symbol-section to match. I believe it is because the
> .section and .global entries order is reversed in this target.
>
> Patch in inlined below. looking forward to your comments.
>
> Cupertino
>
> diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
> index 63363a03b9f..82b4cd88ec0 100644
> --- a/gcc/testsuite/gcc.dg/pr25521.c
> +++ b/gcc/testsuite/gcc.dg/pr25521.c
> @@ -2,9 +2,10 @@
>     sections.
>
>     { dg-require-effective-target elf }
> -   { dg-do compile } */
> +   { dg-do compile }
> +   { dg-skip-if "" { ! const_volatile_readonly_section } } */
>
>  const volatile int foo = 30;
>
> -
> -/* { dg-final { scan-assembler "\\.s\?rodata" } } */
> +/* { dg-final { scan-assembler {.section C,} { target { rx-*-* } } } } */
> +/* { dg-final { scan-assembler-symbol-section {^_?foo$} {^\.(const|s?rodata)} { target { ! "rx-*-*" } } } } */
> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index c0694af2338..91aafd89909 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -12295,3 +12295,13 @@ proc check_is_prog_name_available { prog } {
>
>      return 1
>  }
> +
> +# returns 1 if target does selects a readonly section for const volatile variables.
> +proc check_effective_target_const_volatile_readonly_section { } {
> +
> +    if { [istarget powerpc-*-*]
> +    	  || [check-flags { "" { powerpc64-*-* } { -m32 } }] } {
> +	return 0
> +    }
> +  return 1
> +}
>
>
> Jeff Law writes:
>
>> On 12/7/22 08:45, Cupertino Miranda wrote:
>>>
>>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>>> This commit is a follow up of bugzilla #107181.
>>>>> The commit /a0aafbc/ changed the default implementation of the
>>>>> SELECT_SECTION hook in order to match clang/llvm behaviour w.r.t the
>>>>> placement of `const volatile' objects.
>>>>> However, the following targets use target-specific selection functions
>>>>> and they choke on the testcase pr25521.c:
>>>>>    *rx - target sets its const variables as '.section C,"a",@progbits'.
>>>> That's presumably a constant section.  We should instead twiddle the test to
>>>> recognize that section.
>>> Although @progbits is indeed a constant section, I believe it is
>>> more interesting to detect if the `rx' starts selecting more
>>> standard sections instead of the current @progbits.
>>> That was the reason why I opted to XFAIL instead of PASSing it.
>>> Can I keep it as such ?
>> I'm not aware of any ongoing development for that port, so I would not let
>> concerns about the rx port changing behavior dominate how we approach this
>> problem.
>>
>> The rx port is using a different name for the section.  That's  valid thing to
>> do and to the extent we can, we should support that in the test rather than
>> (incorrectly IMHO) xfailing the test just becuase the name isn't what we
>> expected.
>>
>> To avoid over-eagerly matching, I would probably search for "C,"  I wouldn't do
>> that for the const or rodata sections as they often have a suffix like 1, 2, 4,
>> 8 for different sized rodata sections.
>>
>> PPC32 is explicitly doing something different and placing those objects into an
>> RW section.  So for PPC32 it makes more sense to skip the test rather than xfail
>> it.
>>
>> Jeff
  

Patch

diff --git a/gcc/testsuite/gcc.dg/pr25521.c b/gcc/testsuite/gcc.dg/pr25521.c
index 63363a03b9f..597a2fc25d8 100644
--- a/gcc/testsuite/gcc.dg/pr25521.c
+++ b/gcc/testsuite/gcc.dg/pr25521.c
@@ -6,5 +6,4 @@ 
 
 const volatile int foo = 30;
 
-
-/* { dg-final { scan-assembler "\\.s\?rodata" } } */
+/* { dg-final { scan-assembler-symbol-section {^_?foo$} {^\.(const|s?rodata)} { xfail { ! const_volatile_readonly_section } } } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 2a058c67c53..631d4593447 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -12196,3 +12196,15 @@  proc check_is_prog_name_available { prog } {
 
     return 1
 }
+
+# returns 1 if target does selects a readonly section for const volatile variables.
+proc check_effective_target_const_volatile_readonly_section { } {
+
+    if { [istarget rx*-*-*]
+	  || [istarget powerpc-*-*]
+	  || [istarget rs6000*-*-*]
+    	  || [check-flags { "" { powerpc64-*-* } { -m32 } }] } {
+	return 0
+    }
+  return 1
+}