[PATCH] `const volatile' sections selection - bugzilla #107181

[PATCH] `const volatile' sections selection - bugzilla #107181

[Cupertino Miranda]

Hi everyone,

Recently I looked over the issue reported in bugzilla #107181, related
to commit `a0aafbc'.

The commit changes the object section for `const volatile' objects.
However it does it only for the targets using the default section
selection hook.
The included patches addresses all the architectures that define a custom
section select by verifying its result against pr25521.c test.

From all architectures verified only powerpc and v850 seem to be having
a non-default behaviour and required changes.

- v850_select_section was changed to follow the behaviour the default
  select_section hook.
- powerpc only has issues in 32bit, since the compiler is eager to
  allocate the simple const volatile object in small data section, 
  which does not appear to have a readonly counter-part support.
  Please find more details in the patch commit message.

Looking forward to your review.

Best regards,
Cupertino Miranda

[PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

Changed target code to select .rodata section for 'const volatile'
defined variables.
This change is in the context of the bugzilla #170181.

gcc/ChangeLog:

	v850.c(v850_select_section): Changed function.
---
 gcc/config/v850/v850.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
index c7d432990ab..e66893fede4 100644
--- a/gcc/config/v850/v850.cc
+++ b/gcc/config/v850/v850.cc
@@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
     {
       int is_const;
       if (!TREE_READONLY (exp)
-	  || TREE_SIDE_EFFECTS (exp)
 	  || !DECL_INITIAL (exp)
 	  || (DECL_INITIAL (exp) != error_mark_node
 	      && !TREE_CONSTANT (DECL_INITIAL (exp))))
-- 
2.30.2

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

[Cupertino Miranda]

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

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
+}
-- 
2.30.2

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Jeff Law]

On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
> Changed target code to select .rodata section for 'const volatile'
> defined variables.
> This change is in the context of the bugzilla #170181.
> 
> gcc/ChangeLog:
> 
> 	v850.c(v850_select_section): Changed function.
I'm not sure this is safe/correct.  ISTM that you need to look at the 
underlying TREE_TYPE to check for const-volatile rather than 
TREE_SIDE_EFFECTS.

Of secondary importance is the ChangeLog.  Just saying "Changed 
function" provides no real information.  Something like this would be 
better:

	* config/v850/v850.c (v850_select_section): Put const volatile
	objects into read-only sections.


Jeff




> ---
>   gcc/config/v850/v850.cc | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
> index c7d432990ab..e66893fede4 100644
> --- a/gcc/config/v850/v850.cc
> +++ b/gcc/config/v850/v850.cc
> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>       {
>         int is_const;
>         if (!TREE_READONLY (exp)
> -	  || TREE_SIDE_EFFECTS (exp)
>   	  || !DECL_INITIAL (exp)
>   	  || (DECL_INITIAL (exp) != error_mark_node
>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Jeff Law]

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

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

Hi Jeff,

First of all thanks for your quick review.
Apologies for the delay replying, the message got lost in my inbox.

> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>> Changed target code to select .rodata section for 'const volatile'
>> defined variables.
>> This change is in the context of the bugzilla #170181.
>> gcc/ChangeLog:
>> 	v850.c(v850_select_section): Changed function.
> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.

I believe this was asked by Jose when he first sent the generic patches.
Please notice my change is influenced by his original patch that does
the same and was approved.

https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html

>
> Of secondary importance is the ChangeLog.  Just saying "Changed function"
> provides no real information.  Something like this would be better:
>
> 	* config/v850/v850.c (v850_select_section): Put const volatile
> 	objects into read-only sections.
>
>
> Jeff
>
>
>
>
>> ---
>>   gcc/config/v850/v850.cc | 1 -
>>   1 file changed, 1 deletion(-)
>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>> index c7d432990ab..e66893fede4 100644
>> --- a/gcc/config/v850/v850.cc
>> +++ b/gcc/config/v850/v850.cc
>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>       {
>>         int is_const;
>>         if (!TREE_READONLY (exp)
>> -	  || TREE_SIDE_EFFECTS (exp)
>>   	  || !DECL_INITIAL (exp)
>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

> 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

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

gentle ping

Cupertino Miranda writes:

> Hi Jeff,
>
> First of all thanks for your quick review.
> Apologies for the delay replying, the message got lost in my inbox.
>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> Changed target code to select .rodata section for 'const volatile'
>>> defined variables.
>>> This change is in the context of the bugzilla #170181.
>>> gcc/ChangeLog:
>>> 	v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>
> I believe this was asked by Jose when he first sent the generic patches.
> Please notice my change is influenced by his original patch that does
> the same and was approved.
>
> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>
>>
>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>> provides no real information.  Something like this would be better:
>>
>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>> 	objects into read-only sections.
>>
>>
>> Jeff
>>
>>
>>
>>
>>> ---
>>>   gcc/config/v850/v850.cc | 1 -
>>>   1 file changed, 1 deletion(-)
>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>> index c7d432990ab..e66893fede4 100644
>>> --- a/gcc/config/v850/v850.cc
>>> +++ b/gcc/config/v850/v850.cc
>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>       {
>>>         int is_const;
>>>         if (!TREE_READONLY (exp)
>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>   	  || !DECL_INITIAL (exp)
>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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] Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

Cupertino Miranda via Gcc-patches writes:

> gentle ping
>
> Cupertino Miranda writes:
>
>> Hi Jeff,
>>
>> First of all thanks for your quick review.
>> Apologies for the delay replying, the message got lost in my inbox.
>>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>> Changed target code to select .rodata section for 'const volatile'
>>>> defined variables.
>>>> This change is in the context of the bugzilla #170181.
>>>> gcc/ChangeLog:
>>>> 	v850.c(v850_select_section): Changed function.
>>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>>
>> I believe this was asked by Jose when he first sent the generic patches.
>> Please notice my change is influenced by his original patch that does
>> the same and was approved.
>>
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
>> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>>
>>>
>>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>>> provides no real information.  Something like this would be better:
>>>
>>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>>> 	objects into read-only sections.
>>>
>>>
>>> Jeff
>>>
>>>
>>>
>>>
>>>> ---
>>>>   gcc/config/v850/v850.cc | 1 -
>>>>   1 file changed, 1 deletion(-)
>>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>>> index c7d432990ab..e66893fede4 100644
>>>> --- a/gcc/config/v850/v850.cc
>>>> +++ b/gcc/config/v850/v850.cc
>>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>>       {
>>>>         int is_const;
>>>>         if (!TREE_READONLY (exp)
>>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>>   	  || !DECL_INITIAL (exp)
>>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))

[PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PING] Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

PING PING

Cupertino Miranda writes:

> Cupertino Miranda via Gcc-patches writes:
>
>> gentle ping
>>
>> Cupertino Miranda writes:
>>
>>> Hi Jeff,
>>>
>>> First of all thanks for your quick review.
>>> Apologies for the delay replying, the message got lost in my inbox.
>>>
>>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>>>> Changed target code to select .rodata section for 'const volatile'
>>>>> defined variables.
>>>>> This change is in the context of the bugzilla #170181.
>>>>> gcc/ChangeLog:
>>>>> 	v850.c(v850_select_section): Changed function.
>>>> I'm not sure this is safe/correct.  ISTM that you need to look at the underlying
>>>> TREE_TYPE to check for const-volatile rather than TREE_SIDE_EFFECTS.
>>>
>>> I believe this was asked by Jose when he first sent the generic patches.
>>> Please notice my change is influenced by his original patch that does
>>> the same and was approved.
>>>
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-August/599348.html
>>> https://gcc.gnu.org/pipermail/gcc-patches/2022-September/602374.html
>>>
>>>>
>>>> Of secondary importance is the ChangeLog.  Just saying "Changed function"
>>>> provides no real information.  Something like this would be better:
>>>>
>>>> 	* config/v850/v850.c (v850_select_section): Put const volatile
>>>> 	objects into read-only sections.
>>>>
>>>>
>>>> Jeff
>>>>
>>>>
>>>>
>>>>
>>>>> ---
>>>>>   gcc/config/v850/v850.cc | 1 -
>>>>>   1 file changed, 1 deletion(-)
>>>>> diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>>>> index c7d432990ab..e66893fede4 100644
>>>>> --- a/gcc/config/v850/v850.cc
>>>>> +++ b/gcc/config/v850/v850.cc
>>>>> @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>>>>       {
>>>>>         int is_const;
>>>>>         if (!TREE_READONLY (exp)
>>>>> -	  || TREE_SIDE_EFFECTS (exp)
>>>>>   	  || !DECL_INITIAL (exp)
>>>>>   	  || (DECL_INITIAL (exp) != error_mark_node
>>>>>   	      && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Richard Biener]

On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
> > Changed target code to select .rodata section for 'const volatile'
> > defined variables.
> > This change is in the context of the bugzilla #170181.
> >
> > gcc/ChangeLog:
> >
> >       v850.c(v850_select_section): Changed function.
> I'm not sure this is safe/correct.  ISTM that you need to look at the
> underlying TREE_TYPE to check for const-volatile rather than
> TREE_SIDE_EFFECTS.

Just to quote tree.h:

/* In any expression, decl, or constant, nonzero means it has side effects or
   reevaluation of the whole expression could produce a different value.
   This is set if any subexpression is a function call, a side effect or a
   reference to a volatile variable.  In a ..._DECL, this is set only if the
   declaration said `volatile'.  This will never be set for a constant.  */
#define TREE_SIDE_EFFECTS(NODE) \
  (NON_TYPE_CHECK (NODE)->base.side_effects_flag)

so if exp is a decl then that's the volatile check.

> Of secondary importance is the ChangeLog.  Just saying "Changed
> function" provides no real information.  Something like this would be
> better:
>
>         * config/v850/v850.c (v850_select_section): Put const volatile
>         objects into read-only sections.
>
>
> Jeff
>
>
>
>
> > ---
> >   gcc/config/v850/v850.cc | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
> > index c7d432990ab..e66893fede4 100644
> > --- a/gcc/config/v850/v850.cc
> > +++ b/gcc/config/v850/v850.cc
> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
> >       {
> >         int is_const;
> >         if (!TREE_READONLY (exp)
> > -       || TREE_SIDE_EFFECTS (exp)
> >         || !DECL_INITIAL (exp)
> >         || (DECL_INITIAL (exp) != error_mark_node
> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

Richard Biener writes:

> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>> > Changed target code to select .rodata section for 'const volatile'
>> > defined variables.
>> > This change is in the context of the bugzilla #170181.
>> >
>> > gcc/ChangeLog:
>> >
>> >       v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>> underlying TREE_TYPE to check for const-volatile rather than
>> TREE_SIDE_EFFECTS.
>
> Just to quote tree.h:
>
> /* In any expression, decl, or constant, nonzero means it has side effects or
>    reevaluation of the whole expression could produce a different value.
>    This is set if any subexpression is a function call, a side effect or a
>    reference to a volatile variable.  In a ..._DECL, this is set only if the
>    declaration said `volatile'.  This will never be set for a constant.  */
> #define TREE_SIDE_EFFECTS(NODE) \
>   (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
>
> so if exp is a decl then that's the volatile check.
>

Thank you Richard for the review.
Jeff: Can you please let me know if Richard comments reply to your
concerns?

Cupertino

>> Of secondary importance is the ChangeLog.  Just saying "Changed
>> function" provides no real information.  Something like this would be
>> better:
>>
>>         * config/v850/v850.c (v850_select_section): Put const volatile
>>         objects into read-only sections.
>>
>>
>> Jeff
>>
>>
>>
>>
>> > ---
>> >   gcc/config/v850/v850.cc | 1 -
>> >   1 file changed, 1 deletion(-)
>> >
>> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>> > index c7d432990ab..e66893fede4 100644
>> > --- a/gcc/config/v850/v850.cc
>> > +++ b/gcc/config/v850/v850.cc
>> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>> >       {
>> >         int is_const;
>> >         if (!TREE_READONLY (exp)
>> > -       || TREE_SIDE_EFFECTS (exp)
>> >         || !DECL_INITIAL (exp)
>> >         || (DECL_INITIAL (exp) != error_mark_node
>> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Cupertino Miranda]

Hi Jeff,

Kindly calling your attention to this thread.

Regards,
Cupertino

Cupertino Miranda via Gcc-patches writes:

> Richard Biener writes:
>
>> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>
>>>
>>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> > Changed target code to select .rodata section for 'const volatile'
>>> > defined variables.
>>> > This change is in the context of the bugzilla #170181.
>>> >
>>> > gcc/ChangeLog:
>>> >
>>> >       v850.c(v850_select_section): Changed function.
>>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>>> underlying TREE_TYPE to check for const-volatile rather than
>>> TREE_SIDE_EFFECTS.
>>
>> Just to quote tree.h:
>>
>> /* In any expression, decl, or constant, nonzero means it has side effects or
>>    reevaluation of the whole expression could produce a different value.
>>    This is set if any subexpression is a function call, a side effect or a
>>    reference to a volatile variable.  In a ..._DECL, this is set only if the
>>    declaration said `volatile'.  This will never be set for a constant.  */
>> #define TREE_SIDE_EFFECTS(NODE) \
>>   (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
>>
>> so if exp is a decl then that's the volatile check.
>>
>
> Thank you Richard for the review.
> Jeff: Can you please let me know if Richard comments reply to your
> concerns?
>
> Cupertino
>
>>> Of secondary importance is the ChangeLog.  Just saying "Changed
>>> function" provides no real information.  Something like this would be
>>> better:
>>>
>>>         * config/v850/v850.c (v850_select_section): Put const volatile
>>>         objects into read-only sections.
>>>
>>>
>>> Jeff
>>>
>>>
>>>
>>>
>>> > ---
>>> >   gcc/config/v850/v850.cc | 1 -
>>> >   1 file changed, 1 deletion(-)
>>> >
>>> > diff --git a/gcc/config/v850/v850.cc b/gcc/config/v850/v850.cc
>>> > index c7d432990ab..e66893fede4 100644
>>> > --- a/gcc/config/v850/v850.cc
>>> > +++ b/gcc/config/v850/v850.cc
>>> > @@ -2865,7 +2865,6 @@ v850_select_section (tree exp,
>>> >       {
>>> >         int is_const;
>>> >         if (!TREE_READONLY (exp)
>>> > -       || TREE_SIDE_EFFECTS (exp)
>>> >         || !DECL_INITIAL (exp)
>>> >         || (DECL_INITIAL (exp) != error_mark_node
>>> >             && !TREE_CONSTANT (DECL_INITIAL (exp))))

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Jeff Law]

On 1/19/23 02:59, Cupertino Miranda wrote:
> 
> Hi Jeff,
> 
> Kindly calling your attention to this thread.
Sorry, just crazy busy around here.

Jeff

Re: [PATCH 1/2] select .rodata for const volatile variables.

[Jeff Law]

On 1/9/23 00:57, Richard Biener wrote:
> On Mon, Dec 5, 2022 at 7:07 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 12/2/22 10:52, Cupertino Miranda via Gcc-patches wrote:
>>> Changed target code to select .rodata section for 'const volatile'
>>> defined variables.
>>> This change is in the context of the bugzilla #170181.
>>>
>>> gcc/ChangeLog:
>>>
>>>        v850.c(v850_select_section): Changed function.
>> I'm not sure this is safe/correct.  ISTM that you need to look at the
>> underlying TREE_TYPE to check for const-volatile rather than
>> TREE_SIDE_EFFECTS.
> 
> Just to quote tree.h:
> 
> /* In any expression, decl, or constant, nonzero means it has side effects or
>     reevaluation of the whole expression could produce a different value.
>     This is set if any subexpression is a function call, a side effect or a
>     reference to a volatile variable.  In a ..._DECL, this is set only if the
>     declaration said `volatile'.  This will never be set for a constant.  */
> #define TREE_SIDE_EFFECTS(NODE) \
>    (NON_TYPE_CHECK (NODE)->base.side_effects_flag)
> 
> so if exp is a decl then that's the volatile check.
Ah, then we can just use TREE_SIDE_EFFECTS for testing if a _DECL node 
is volatile.  So that concern is a non-issue.  I think that was the only 
concern with patch #1.  I'll install it momentarily.

Jeff

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Jeff Law]

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

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: a.patch --]
[-- Type: text/x-diff, Size: 1240 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 1762 bytes --]



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] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

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

Re: [PING, PING] Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

[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

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Jeff Law]

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

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]


> 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-item-entry-in-docs-for-the-new-check_effective.patch --]
[-- Type: text/x-diff, Size: 896 bytes --]

From ea2a0434014516c7f680d24f0e702b407be1b83e Mon Sep 17 00:00:00 2001
From: Cupertino Miranda <cupertino.miranda@oracle.com>
Date: Mon, 13 Mar 2023 17:45:14 +0000
Subject: [PATCH] Added item entry in docs for the new check_effective_target.

More precisely for:
 - check_effective_target_const_volatile_readonly_section
---
 gcc/doc/sourcebuild.texi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index be4318221cc..c614292f864 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2777,6 +2777,9 @@ Target supports @option{-branch-cost=N}.
 @item cxa_atexit
 Target uses @code{__cxa_atexit}.
 
+@item const_volatile_readonly_section
+Target places const volatile variables in readonly sections.
+
 @item default_packed
 @anchor{default_packed}
 Target has packed layout of structure members by default.
-- 
2.38.1

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Cupertino Miranda]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]


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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Added-item-entry-in-docs-for-the-new-check_effective.patch --]
[-- Type: text/x-diff, Size: 855 bytes --]

From 2a4f87294ad0af037b80e13d67678e52bc382c95 Mon Sep 17 00:00:00 2001
From: Cupertino Miranda <cupertino.miranda@oracle.com>
Date: Mon, 13 Mar 2023 17:45:14 +0000
Subject: [PATCH] Added item entry in docs for the new check_effective_target.

More precisely for:
 - check_effective_target_const_volatile_readonly_section
---
 gcc/doc/sourcebuild.texi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index be4318221cc..f08893aeb02 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2774,6 +2774,9 @@ Target supports automatic stack alignment.
 @item branch_cost
 Target supports @option{-branch-cost=N}.
 
+@item const_volatile_readonly_section
+Target places const volatile variables in readonly sections.
+
 @item cxa_atexit
 Target uses @code{__cxa_atexit}.
 
-- 
2.38.1

Re: [PATCH 2/2] Corrected pr25521.c target matching.

[Jeff Law]

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