[PATCH][GCC] Make stackalign test LTO proof

[PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

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

Hi,

   This patch changes this testcase to make sure LTO will not optimize 
away the assignment of the local array to a global variable which was 
introduced to make sure stack space was made available for the test to work.

   This is correct because LTO is supposed to optimize this global away 
as at link time it knows this global will never be read. By adding a 
read of the global, LTO will no longer optimize it away.

   Tested by running regressions for this testcase for various ARM targets.

   Is this OK to commit?

   Thanks,
   Andre Vieira

gcc/testsuite/ChangeLog:
2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>

         * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
           to global such that a write is not optimized away by LTO.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-keep-the-stack-testsuite-fix.patch --]
[-- Type: text/x-patch; name=0001-keep-the-stack-testsuite-fix.patch, Size: 977 bytes --]

From 6fbac447475c3b669baee84aa9d6291c3d09f1ab Mon Sep 17 00:00:00 2001
From: Andre Simoes Dias Vieira <andsim01@arm.com>
Date: Fri, 6 Nov 2015 13:13:47 +0000
Subject: [PATCH] keep the stack testsuite fix

---
 gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
index af017532aeb3878ef7ad717a2743661a87a56b7d..1ccd109256de72419a3c71c2c1be6d07c423c005 100644
--- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
+++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
@@ -39,5 +39,10 @@ int main(void)
   if (bar(1) != 2)
     abort();
 
+  /* Make sure there is a read of the global after the function call to bar
+   * such that LTO does not optimize away the assignment above.  */
+  if (g != dummy)
+    abort();
+
   return 0;
 }
-- 
1.9.1

Re: [PATCH][GCC] Make stackalign test LTO proof

[Richard Biener]

On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira
<Andre.SimoesDiasVieira@arm.com> wrote:
> Hi,
>
>   This patch changes this testcase to make sure LTO will not optimize away
> the assignment of the local array to a global variable which was introduced
> to make sure stack space was made available for the test to work.
>
>   This is correct because LTO is supposed to optimize this global away as at
> link time it knows this global will never be read. By adding a read of the
> global, LTO will no longer optimize it away.

But that's only because we can't see that bar doesn't clobber it, else
we would optimize away the check and get here again.  Much better
to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely.

Richard.

>   Tested by running regressions for this testcase for various ARM targets.
>
>   Is this OK to commit?
>
>   Thanks,
>   Andre Vieira
>
> gcc/testsuite/ChangeLog:
> 2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>
>         * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
>           to global such that a write is not optimized away by LTO.

Re: [PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

On 13/11/15 10:34, Richard Biener wrote:
> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira
> <Andre.SimoesDiasVieira@arm.com> wrote:
>> Hi,
>>
>>    This patch changes this testcase to make sure LTO will not optimize away
>> the assignment of the local array to a global variable which was introduced
>> to make sure stack space was made available for the test to work.
>>
>>    This is correct because LTO is supposed to optimize this global away as at
>> link time it knows this global will never be read. By adding a read of the
>> global, LTO will no longer optimize it away.
>
> But that's only because we can't see that bar doesn't clobber it, else
> we would optimize away the check and get here again.  Much better
> to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely.
>
> Richard.
>
>>    Tested by running regressions for this testcase for various ARM targets.
>>
>>    Is this OK to commit?
>>
>>    Thanks,
>>    Andre Vieira
>>
>> gcc/testsuite/ChangeLog:
>> 2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>
>>          * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
>>            to global such that a write is not optimized away by LTO.
>
Hi Richard,

That would be great but __attribute__ ((used)) can only be used for 
static variables and making dummy static would defeat the purpose here.

Cheers,
Andre

Re: [PATCH][GCC] Make stackalign test LTO proof

[Richard Biener]

On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira
<Andre.SimoesDiasVieira@arm.com> wrote:
> On 13/11/15 10:34, Richard Biener wrote:
>>
>> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira
>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>
>>> Hi,
>>>
>>>    This patch changes this testcase to make sure LTO will not optimize
>>> away
>>> the assignment of the local array to a global variable which was
>>> introduced
>>> to make sure stack space was made available for the test to work.
>>>
>>>    This is correct because LTO is supposed to optimize this global away
>>> as at
>>> link time it knows this global will never be read. By adding a read of
>>> the
>>> global, LTO will no longer optimize it away.
>>
>>
>> But that's only because we can't see that bar doesn't clobber it, else
>> we would optimize away the check and get here again.  Much better
>> to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely.
>>
>> Richard.
>>
>>>    Tested by running regressions for this testcase for various ARM
>>> targets.
>>>
>>>    Is this OK to commit?
>>>
>>>    Thanks,
>>>    Andre Vieira
>>>
>>> gcc/testsuite/ChangeLog:
>>> 2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>>
>>>          * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
>>>            to global such that a write is not optimized away by LTO.
>>
>>
> Hi Richard,
>
> That would be great but __attribute__ ((used)) can only be used for static
> variables and making dummy static would defeat the purpose here.

I see.  What about volatile?

> Cheers,
> Andre
>

Re: [PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

On 16/11/15 13:33, Richard Biener wrote:
> On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira
> <Andre.SimoesDiasVieira@arm.com> wrote:
>> On 13/11/15 10:34, Richard Biener wrote:
>>>
>>> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira
>>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>>     This patch changes this testcase to make sure LTO will not optimize
>>>> away
>>>> the assignment of the local array to a global variable which was
>>>> introduced
>>>> to make sure stack space was made available for the test to work.
>>>>
>>>>     This is correct because LTO is supposed to optimize this global away
>>>> as at
>>>> link time it knows this global will never be read. By adding a read of
>>>> the
>>>> global, LTO will no longer optimize it away.
>>>
>>>
>>> But that's only because we can't see that bar doesn't clobber it, else
>>> we would optimize away the check and get here again.  Much better
>>> to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely.
>>>
>>> Richard.
>>>
>>>>     Tested by running regressions for this testcase for various ARM
>>>> targets.
>>>>
>>>>     Is this OK to commit?
>>>>
>>>>     Thanks,
>>>>     Andre Vieira
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>> 2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>>>
>>>>           * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
>>>>             to global such that a write is not optimized away by LTO.
>>>
>>>
>> Hi Richard,
>>
>> That would be great but __attribute__ ((used)) can only be used for static
>> variables and making dummy static would defeat the purpose here.
>
> I see.  What about volatile?
>
>> Cheers,
>> Andre
>>
>
Yeh a 'volatile char dummy[64];' with a read afterwards leads to the 
stack still being reserved after LTO and we won't need the global variable.

If you prefer that solution Ill respin the patch.

Cheers,
Andre

Re: [PATCH][GCC] Make stackalign test LTO proof

[Richard Biener]

On Mon, Nov 16, 2015 at 3:08 PM, Andre Vieira
<Andre.SimoesDiasVieira@arm.com> wrote:
> On 16/11/15 13:33, Richard Biener wrote:
>>
>> On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira
>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>
>>> On 13/11/15 10:34, Richard Biener wrote:
>>>>
>>>>
>>>> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira
>>>> <Andre.SimoesDiasVieira@arm.com> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>     This patch changes this testcase to make sure LTO will not optimize
>>>>> away
>>>>> the assignment of the local array to a global variable which was
>>>>> introduced
>>>>> to make sure stack space was made available for the test to work.
>>>>>
>>>>>     This is correct because LTO is supposed to optimize this global
>>>>> away
>>>>> as at
>>>>> link time it knows this global will never be read. By adding a read of
>>>>> the
>>>>> global, LTO will no longer optimize it away.
>>>>
>>>>
>>>>
>>>> But that's only because we can't see that bar doesn't clobber it, else
>>>> we would optimize away the check and get here again.  Much better
>>>> to mark 'dummy' with __attribute__((used)) and go away with 'g'
>>>> entirely.
>>>>
>>>> Richard.
>>>>
>>>>>     Tested by running regressions for this testcase for various ARM
>>>>> targets.
>>>>>
>>>>>     Is this OK to commit?
>>>>>
>>>>>     Thanks,
>>>>>     Andre Vieira
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>> 2015-11-06  Andre Vieira  <andre.simoesdiasvieira@arm.com>
>>>>>
>>>>>           * gcc.dg/torture/stackalign/builtin-return-1.c: Added read
>>>>>             to global such that a write is not optimized away by LTO.
>>>>
>>>>
>>>>
>>> Hi Richard,
>>>
>>> That would be great but __attribute__ ((used)) can only be used for
>>> static
>>> variables and making dummy static would defeat the purpose here.
>>
>>
>> I see.  What about volatile?
>>
>>> Cheers,
>>> Andre
>>>
>>
> Yeh a 'volatile char dummy[64];' with a read afterwards leads to the stack
> still being reserved after LTO and we won't need the global variable.
>
> If you prefer that solution Ill respin the patch.

Yes please.

Richard.

> Cheers,
> Andre
>

Re: [PATCH][GCC] Make stackalign test LTO proof

[Bernd Schmidt]

On 12/04/2015 04:18 PM, Andre Vieira wrote:
> Reworked following Joern's suggestion.
>
> Is this OK?

Yes.


Bernd

Re: [PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

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

On 17/11/15 16:30, Andre Vieira wrote:
> On 17/11/15 12:29, Bernd Schmidt wrote:
>> On 11/16/2015 04:48 PM, Andre Vieira wrote:
>>> On 16/11/15 15:34, Joern Wolfgang Rennecke wrote:
>>>> I just happened to stumble on this problem with another port.
>>>> The volatile & test solution doesn't work, though.
>>>>
>>>> What does work, however, is:
>>>>
>>>> __asm__ ("" : : "" (dummy));
>>>>
>>> I can confirm that Joern's solution works for me too.
>>
>> Ok to make that change.
>>
>>
>> Bernd
>>
> OK, Joern will you submit a patch for this or shall I?
>
> Cheers,
> Andre
>
Hi,

Reworked following Joern's suggestion.

Is this OK?

Cheers,
Andre

gcc/testsuite/ChangeLog:
2015-12-04  Andre Vieira  <andre.simoesdiasvieira@arm.com>
             Joern Rennecke  <joern.rennecke@embecosm.com>

         * gcc.dg/torture/stackalign/builtin-return-1.c: Add an
           inline assembly read to make sure dummy is not optimized
           away by LTO.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fixed-test-to-be-LTO-proof.patch --]
[-- Type: text/x-patch; name=0001-Fixed-test-to-be-LTO-proof.patch, Size: 1070 bytes --]

From 9cee0251e69c1061a60caaf28da598eab2a1fbee Mon Sep 17 00:00:00 2001
From: Andre Simoes Dias Vieira <andsim01@arm.com>
Date: Tue, 24 Nov 2015 13:50:15 +0000
Subject: [PATCH] Fixed test to be LTO proof.

---
 gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
index af017532aeb3878ef7ad717a2743661a87a56b7d..ec4fd8a9ef33a5e755bdb33e4faa41cab0f16a60 100644
--- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
+++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c
@@ -26,15 +26,13 @@ int bar(int n)
 				   STACK_ARGUMENTS_SIZE));
 }
 
-char *g;
-
 int main(void)
 {
   /* Allocate 64 bytes on the stack to make sure that __builtin_apply
      can read at least 64 bytes above the return address.  */
   char dummy[64];
 
-  g = dummy;
+  __asm__ ("" : : "" (dummy));
 
   if (bar(1) != 2)
     abort();
-- 
1.9.1

Re: [PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

On 17/11/15 12:29, Bernd Schmidt wrote:
> On 11/16/2015 04:48 PM, Andre Vieira wrote:
>> On 16/11/15 15:34, Joern Wolfgang Rennecke wrote:
>>> I just happened to stumble on this problem with another port.
>>> The volatile & test solution doesn't work, though.
>>>
>>> What does work, however, is:
>>>
>>> __asm__ ("" : : "" (dummy));
>>>
>> I can confirm that Joern's solution works for me too.
>
> Ok to make that change.
>
>
> Bernd
>
OK, Joern will you submit a patch for this or shall I?

Cheers,
Andre

Re: [PATCH][GCC] Make stackalign test LTO proof

[Bernd Schmidt]

On 11/16/2015 04:48 PM, Andre Vieira wrote:
> On 16/11/15 15:34, Joern Wolfgang Rennecke wrote:
>> I just happened to stumble on this problem with another port.
>> The volatile & test solution doesn't work, though.
>>
>> What does work, however, is:
>>
>> __asm__ ("" : : "" (dummy));
>>
> I can confirm that Joern's solution works for me too.

Ok to make that change.


Bernd

Re: [PATCH][GCC] Make stackalign test LTO proof

[Andre Vieira]

On 16/11/15 15:34, Joern Wolfgang Rennecke wrote:
> I just happened to stumble on this problem with another port.
> The volatile & test solution doesn't work, though.
>
> What does work, however, is:
>
> __asm__ ("" : : "" (dummy));
>
I can confirm that Joern's solution works for me too.

Re: [PATCH][GCC] Make stackalign test LTO proof

[Joern Wolfgang Rennecke]

I just happened to stumble on this problem with another port.
The volatile & test solution doesn't work, though.

What does work, however, is:

__asm__ ("" : : "" (dummy));