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

  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(-)
  

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
  

> 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
  

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 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
  

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 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
  

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
  

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 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
  

Hi Jeff,

Can you please confirm if the patch is Ok?

Thanks,
Cupertino

> 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
  

PING !!!!!

Cupertino Miranda via Gcc-patches writes:

> Hi Jeff,
>
> Can you please confirm if the patch is Ok?
>
> Thanks,
> Cupertino
>
>> 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
  

Hi Jeff,

Please, please, give me some feedback on this one.
I just don't want to have to keep asking you for time on this small
pending patches that I also have to keep track on.

I realized your committed the other one. Thank you !

Best regards,
Cupertino


Cupertino Miranda writes:

> PING !!!!!
>
> Cupertino Miranda via Gcc-patches writes:
>
>> Hi Jeff,
>>
>> Can you please confirm if the patch is Ok?
>>
>> Thanks,
>> Cupertino
>>
>>> 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
  

[PING]

Cupertino Miranda writes:

> Hi Jeff,
>
> Please, please, give me some feedback on this one.
> I just don't want to have to keep asking you for time on this small
> pending patches that I also have to keep track on.
>
> I realized your committed the other one. Thank you !
>
> Best regards,
> Cupertino
>
>
> Cupertino Miranda writes:
>
>> PING !!!!!
>>
>> Cupertino Miranda via Gcc-patches writes:
>>
>>> Hi Jeff,
>>>
>>> Can you please confirm if the patch is Ok?
>>>
>>> Thanks,
>>> Cupertino
>>>
>>>> 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
  

On 1/24/23 05:24, Cupertino Miranda wrote:
> 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.
Sorry for the long delays.   I've installed this version.

As a follow-up, can you update the documentation in doc/sourcebuild.texi 
to include the new check-effective-target test?

Thanks,
Jeff
  

> On 1/24/23 05:24, Cupertino Miranda wrote:
>> 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.
> Sorry for the long delays.   I've installed this version.
>
> As a follow-up, can you update the documentation in doc/sourcebuild.texi to
> include the new check-effective-target test?

Hi Jeff,

Thank you for installing the patch.
I have prepared the doc change you requested.
Hopefully this is what you were expecting.

Regards,
Cupertino
  

Cupertino Miranda via Gcc-patches writes:

>> On 1/24/23 05:24, Cupertino Miranda wrote:
>>> 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.
>> Sorry for the long delays.   I've installed this version.
>>
>> As a follow-up, can you update the documentation in doc/sourcebuild.texi to
>> include the new check-effective-target test?
>
> Hi Jeff,
>
> Thank you for installing the patch.
> I have prepared the doc change you requested.
> Hopefully this is what you were expecting.
>
> Regards,
> Cupertino

Just realized previous patch was in incorrect placement alphabetically.
Please consider this one instead.

Cupertino
  

On 3/13/23 11:57, Cupertino Miranda wrote:
> 
> Cupertino Miranda via Gcc-patches writes:
> 
>>> On 1/24/23 05:24, Cupertino Miranda wrote:
>>>> 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.
>>> Sorry for the long delays.   I've installed this version.
>>>
>>> As a follow-up, can you update the documentation in doc/sourcebuild.texi to
>>> include the new check-effective-target test?
>>
>> Hi Jeff,
>>
>> Thank you for installing the patch.
>> I have prepared the doc change you requested.
>> Hopefully this is what you were expecting.
>>
>> Regards,
>> Cupertino
> 
> Just realized previous patch was in incorrect placement alphabetically.
> Please consider this one instead.
Installed.  Thanks.
jeff